-
Notifications
You must be signed in to change notification settings - Fork 68
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 & unit tested store_dft_output #123
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.
Looks good, just made some minor suggestions on documentation.
from flare.util import is_std_in_bound | ||
|
||
|
||
class OTF: | ||
"""Trains a Gaussian process force field on the fly. | ||
"""Trains a Gaussian process force field on the fly while running a | ||
molecular dynamics simulation. | ||
|
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.
Slightly more succinct: "...on the fly during molecular dynamics."
flare/otf.py
Outdated
@@ -61,6 +64,11 @@ class OTF: | |||
"srun". | |||
dft_kwargs ([type], optional): Additional DFT arguments. Defaults | |||
to 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.
Unrelated, but would it be possible to add some more information about dft_kwargs to the docstring?
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.
Done deal.
copy the file or files specified in the first element of the tuple | ||
to a directory specified as the second element of the tuple. | ||
Useful when DFT calculations are expensive and want to be kept | ||
for later use. | ||
""" |
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.
Is the first element of the tuple a list of strings? Or just a string? It looks like you've set it up to work with both, but it might be helpful to explicitly state that in the docstring. (You make this very clear in the constructor by giving the type as "Tuple[Union[str,List[str]],str]". Consider doing the same here, so that it shows up on RTD.)
dest = self.store_dft_output[1] | ||
target_files = self.store_dft_output[0] | ||
now = datetime.now() | ||
dt_string = now.strftime("%Y.%m.%d:%H:%M:%S:") |
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.
This seems like a good way of labeling the output files. We should write the labeling convention in the docstring for the "run" method.
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 have added the requested changes in a new commit. Thanks, Jon!
Codecov Report
@@ Coverage Diff @@
## master #123 +/- ##
=========================================
+ Coverage 59.19% 59.29% +0.1%
=========================================
Files 31 31
Lines 4749 4761 +12
=========================================
+ Hits 2811 2823 +12
Misses 1938 1938
Continue to review full report at Codecov.
|
Closes #122