Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding LambdaSettings object for AFE #681
Adding LambdaSettings object for AFE #681
Changes from 9 commits
3b221ad
fff204f
95ee8b7
3e6e73b
75e36ea
d6f4b70
4def050
bf4d3b0
2bc510b
8c94a91
d915504
babfa16
774af86
ef55cc9
3a1a177
9c07d76
7e44518
ad51ce8
cf56f51
3d843eb
2694992
03c137a
2ecbcc7
a82a679
29cc6d0
ecee8a1
58c7ba8
afd7a41
e497cf5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Maybe add more details about what the settings class does?
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.
There may be a better way to compare different list lengths here?
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 validator currently would lead to errors when changing the settings from the default settings (since when changing
lambda_elec
the list oflambda_vdw
has still the old length and therefore would raise an error. One could move the validator into the protocol instead, or do you have other suggestions @richardjgowers ?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.
Yeah there might be a limit to what's possible in the
@validator
and instead you want a protocol function. The nice thing about these (when possible) is it fails fast & early.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 moved the check to the
_get_lambda_schedule
function, or would it be better to have a separate_validator
function there?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.
Would it be reasonable here to change the default to match what we usually run as a default when doing AHFEs, i.e. 14 windows?
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.
Ah yes, had changed that in the other PR, but not here, added this now!
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.
Or something like that.