-
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
10 add free energy perturbation parameters to md workflow #14
10 add free energy perturbation parameters to md workflow #14
Conversation
61ac073
to
67fb972
Compare
I'll drop a review later today. |
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.
Very good. I left some couple of minor doubts for @ladinesa regarding syntax.
Overall, I think we could start using more SOLID design ideas, particularly because you are doing it when having defined Property
.
My suggestion would be that maybe Bernadette can take this as an initial project for learning the software? I don't know, it might be annoying to handle these suggestions, and you might decide not to go for these changes 🙂
@@ -504,6 +662,10 @@ class MolecularDynamicsMethod(SimulationWorkflowMethod): | |||
|
|||
barostat_parameters = SubSection(sub_section=BarostatParameters.m_def, repeats=True) | |||
|
|||
free_energy_calculation_parameters = SubSection( |
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.
Coming to my comment on "note for the future": if you would have defined BaseParameters
, then here you could substitute thermostat_parameters
, barostat_parameters
, free_energy_calculation_parameters
by parameters = SubSection(sub_section=BaseParameters.m_def, repeats=True)
, and make sure to define BaseParameters.name = MEnum("thermostat", "barostat", "free_energy_calculation")
.
This is a different design idea (does not need to be better or worse than what is written here), and you don't need to implement it here if you don't want. It is similar to the discussion we had about base classes and defining NumericalSettings
instead of several subsections for describing meshes, basis sets, sfc parameters.
type=np.float64, | ||
shape=["n_frames"], | ||
description=""" | ||
Values of the property. | ||
""", | ||
) | ||
|
||
value_unit = Quantity( |
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 see. I had a similar problem when defining Spectra
. Maybe BaseProperty
should have this by default? We should talk this with @lauri-codes
m_def = Section(validate=False) | ||
|
||
method_ref = Quantity( | ||
type=Reference(FreeEnergyCalculationParameters.m_def), |
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 this syntax the same as type=FreeEnergyCalculationParameters
, @ladinesa ?
""", | ||
) | ||
|
||
value_unit = Quantity( |
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 think that if you elevate this to BaseProperty
, you can also define the description as in here, or, better define each value units separately? Tho I admit is more annoying.
@@ -1039,6 +1294,10 @@ class MolecularDynamicsResults(ThermodynamicsResults): | |||
sub_section=MeanSquaredDisplacement, repeats=True | |||
) | |||
|
|||
free_energy_calculations = SubSection( |
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 silly question, related with the English and which is also bothering me when defining the base classes: if this is a singular MSection, shouldn't we be using singular? Then, the SubSection, if repeated, is in plural. Like (note the singular and plural):
free_energy_calculations = SubSection(FreeEnergyCalculation, repeats=True)
Or, if it does not repeat:
free_energy_calculation = SubSection(FreeEnergyCalculation)
I am just going to merge this and then create a new issue + branch to refactor parameters and properties as suggested by @JosePizarro3 above |
@JosePizarro3 This is my first draft of a schema for the FE calculation workflow that I mentioned to you. Could you take a look sometime? Also, I just wanted you to be aware of any schema additions just in case.