-
Notifications
You must be signed in to change notification settings - Fork 10
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
base: main
Are you sure you want to change the base?
Xtb contribution #711
Conversation
There was a problem hiding this 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.
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) |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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?
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 |
There was a problem hiding this comment.
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?
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') |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
Added xTB calculator.