Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Xtb contribution #711

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Xtb contribution #711

wants to merge 11 commits into from

Conversation

mgt16-LANL
Copy link

Added xTB calculator.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2023

Binder 👈 Launch a binder notebook on branch mgt16-LANL/pyiron_contrib/xtb_contribution

Copy link
Contributor

@pmrv pmrv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! I've left some nits below, but looks generally good.

Comment on lines 100 to +101
def from_hdf(self, hdf=None, group_name=None):
super(FitsnapJob, self).from_hdf(hdf=hdf, group_name=group_name)
super(FitsnapJob, self).from_hdf(hdf=hdf, group_name=group_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see this in the original PR, but this shouldn't be necessary if you're just calling super() anyway. Is there something weird going on, that made it necessary? If so we should look closer to the source.

from xtb.ase.calculator import XTB

@contextlib.contextmanager
def make_temp_directory(prefix=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not use TemporyDirectory?

Comment on lines +54 to +61
outstring = '{}\n\n'.format(len(ase_atoms))
for atom in ase_atoms:
outstring += '{} {} {} {}\n'.format(atom.symbol,
atom.position[0],
atom.position[1],
atom.position[2])
outstring = outstring.strip('\n')
return outstring
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using StringIO and ase_atoms.write() should work as well, or are you specifically trying to surpress the cell dimensions in the output?

Comment on lines +146 to +149
if (self['output'] is not None) and ('xtb' in self['output'].list_groups()) and \
(self.resultsdf.shape[0] == 0):
self.resultsdf = pd.read_hdf(self.project_hdf5.file_name,
key=self.job_name+'/output/xtb')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think writing/reading dataframes directly to hdf should work, but disregard if you've tried that.

ase_atoms.get_total_energy()
except:
pass
if len(ase_atoms.calc.results) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should set the job.status.aborted in the else clause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants