-
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
Added MeamFit code to pyiron_contrib #376
Conversation
Pull Request Test Coverage Report for Build 3036104244
💛 - Coveralls |
I'll be back next week and have a detailed look then, but it already looks fine. |
You still have to add the code to the |
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 @usaikia for moving it over, I think it'll be very useful!
Sorry for the nitty comments, but I'd like new code additions to be pretty good shape otherwise we'll never get around to it.
class MeamFit(GenericJob): | ||
def __init__(self, project, job_name): | ||
""" | ||
ExampleJob generating a list of random numbers to simulate energy fluctuations. |
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.
Needs to be updated. Ideally it would be a little example how to call everything.
time_step_start (int): initial timestep - after equilibration | ||
time_step_end (int): last timestep to use | ||
time_step_delta (int): | ||
quantity (str): ['E', 'F'] for fitting the energies use 'E' and 'F' for fitting the forces |
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.
Why is the default not one of the allowed values?
time_step_end (int): last timestep to use | ||
time_step_delta (int): | ||
quantity (str): ['E', 'F'] for fitting the energies use 'E' and 'F' for fitting the forces | ||
weight (list): default is one, but for lower temperatures use higher weights [100, 1000] |
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.
Why is the default of length three? What does it mean to use a weight >100-1000 if the default is 1? A link to the official documentation would be enough.
def _copy_vasprun_xml(self, cwd=None): | ||
for job_id in self._calculation_dataframe.index: | ||
self.project.load(job_id).decompress() | ||
working_directory = self.project.get_job_working_directory(int(job_id)) | ||
shutil.copyfile(posixpath.join(working_directory, 'vasprun.xml'), | ||
posixpath.join(cwd, self._get_vasprun_name(job_id))) | ||
self.project.load(job_id).compress() |
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.
Move closer to write_input
or even inline it.
Co-authored-by: Marvin Poul <[email protected]>
Co-authored-by: Marvin Poul <[email protected]>
Co-authored-by: Marvin Poul <[email protected]>
Co-authored-by: Marvin Poul <[email protected]>
Co-authored-by: Marvin Poul <[email protected]>
Co-authored-by: Marvin Poul <[email protected]>
Add doc string and runtime error to from_directory(). Also fixed a small bug in _collect_potential_performance() which was also causing problem in collecting outputs.
@pmrv This pull request is complete from my side. Please have a look. If you agree, I will merge it. |
write ‘Fr’ or ‘Fo’ for the second and third cases respectively. | ||
weight (list): default is [1.0, 0.0, 0.0]. | ||
|
||
More information on these parameters are available in the MeamFit user manual- https://www.scd.stfc.ac.uk/Pages/MEAMfit-v2.aspx |
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 cannot find a manual under the given link. Is it not public?
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.
The manual is not available publicly. In order to use the code users has to ask for permission individually from Andrew Ian Duff ([email protected]). Andrew send the code and manual to the user with permission to use the code.
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.
Ok, then let's just put a comment to that effect into the main docstring of the class.
I've made some small changes. @usaikia Can you double check everything still works? Except for the manual link everything seems good to go for me. If the manual is not available publically, just remove the link and merge. |
There should also be a reference to the MEAMFit paper: https://www.sciencedirect.com/science/article/pii/S0010465515001964#f000020
|
remove link to manual as it is not available publicly and add reference to MeamFit paper.
MeamFit was available only in pyiron_mpie. Here I coped the code the from pyiron_mpie and added into pyiron_contrib with a minor modification to copy vasprun.xml files from previously finished vasp md jobs.