-
Notifications
You must be signed in to change notification settings - Fork 21
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
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-02-01 17:40:36 UTC |
@dwhswenson we're not 100% sure how the settings docs is meant to work - do you have a better idea here? We can add the relevant autopydantic entry under docs, but it looks like |
@IAlibay : could you clarify on the settings docs issue? I don't see anything wrong. This PR's docs build for You'll see that this PR's build doesn't have a link for |
Let's add it to our topics of discussion later today - there's a couple of things I'm not 100% sure but it'll be easier to go through in person. |
Validators discussed at today's call:
(for AFE) |
raise ValueError(errmsg) | ||
return v | ||
|
||
@validator('lambda_elec') |
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 of lambda_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.
Regarding our discussion on documentation yesterday, could you add autopydantic entries for LambdaSettings here https://github.com/OpenFreeEnergy/openfe/blob/main/docs/reference/api/openmm_rfe.rst and https://github.com/OpenFreeEnergy/openfe/blob/main/docs/reference/api/openmm_solvation_afe.rst please?
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 here; some docs and a couple of validation things. Otherwise this is looking good!
@@ -404,8 +404,9 @@ def _default_settings(cls): | |||
solvent_system_settings=SystemSettings(), | |||
vacuum_system_settings=SystemSettings(nonbonded_method='nocutoff'), | |||
alchemical_settings=AlchemicalSettings(), | |||
lambda_settings=LambdaSettings(), |
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!
@@ -535,6 +588,9 @@ def _create( | |||
) | |||
self._validate_alchemical_components(alchem_comps) | |||
|
|||
# Validate the lambda schedule | |||
self._validate_lambda_schedule(self.settings) |
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.
To improve method reproducibility, I would switch this to passing self.settings.lambda_settings
instead. That way when we make derived classes we can import the staticmethod across to different places and not face typing errors because we're using different protocol settings.
|
||
Raises | ||
------ | ||
ValueError |
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.
We should also check that there are any non-zero restraints and raise a logger warning if there are (i.e. this protocol doesn't apply restraints, restraints won't be applied).
if v <= 0: | ||
errmsg = ("Number of lambda steps must be positive ") | ||
class LambdaSettings(SettingsBaseModel): | ||
"""Settings for lambda schedule |
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?
Defines lists of floats to control various aspects of the alchemical transformation.
Notes
--------
* In all cases a lambda value of 0 defines a fully interacting state A and a non-interacting state B, whilst a value of 1 defines a fully interacting state B and a non-interacting state A.
* ``lambda_elec``, `lambda_vdw``, and ``lambda_restraints`` must all be of the same length, defining all the windows of the transformation.
This describes the lambda schedule and the creation of the | ||
hybrid system. | ||
"""Settings for the lambda protocol | ||
This describes the lambda schedule. |
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 describes the lambda schedule. | |
Lambda schedule settings. | |
Settings controlling the lambda schedule, these include the switching function type, and the number of windows. |
Or something like that.
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 is amazing, thanks!
Developers certificate of origin