-
Notifications
You must be signed in to change notification settings - Fork 0
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
Removes output files from results if they should not exist #42
Conversation
Hello @hannahbaumann! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-09-26 14:43:29 UTC |
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.
Couple of things, otherwise it looks good.
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.
Two small things, otherwise lgtm!
@@ -178,11 +202,11 @@ def get_gro_filename(self) -> list[pathlib.Path]: | |||
pus[0].outputs["system_gro"] | |||
for pus in self.data.values() | |||
if "GromacsMDSetupUnit" in pus[0].source_key | |||
] | |||
][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.
Are these singular / you take the first element because the expectation is that the input gro file should be the same for all repeat?
If so:
- Could you write a comment about it somewhere?
- Could you update the docstring to not say "llist of paths" for here and I think also
get_top_filename
?
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.
Yes, only a single filepath will be returned as the SetupUnit is only run once, even with multiple repeats. I updated the docstring to clarify this.
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 SetupUnit is only run once
Not to be fixed here, and I'll have to think about this one a bit more, I'm not sure that this doesn't break the model for gufe generations (i.e. extensions). Something to keep in mind in the future.
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.
Just a couple of typos otherwise lgtm!
Co-authored-by: Irfan Alibay <[email protected]>
Co-authored-by: Irfan Alibay <[email protected]>
Co-authored-by: Irfan Alibay <[email protected]>
Description
If the user decides to e.g. not run an energy minimization, the ProtocolResults would still return the filenames to the energy minimization outputs, even though they would not exist. This PR only stores files if the output files would be expected. This does NOT check if the files indeed exist, I'm not sure yet if that would be better or not.
This PR addresses following issues:
#27 : Remove filepaths from outputs if the files are not expected to be created
#34 : Remove stub files