Skip to content

Cookbook on exploring common RBFE protocol settings - #344

Open
jthorton wants to merge 9 commits into
mainfrom
hytop_settings
Open

jthorton wants to merge 9 commits into
mainfrom
hytop_settings

Conversation

@jthorton

Copy link
Copy Markdown
Contributor

Docs in response to OpenFreeEnergy/openfe#2278 this cookbook just explores the common settings users might want to change in the RBFE protocol.

Questions

  • Is this quite simple format okay to start with?
  • Are there any other settings we should add in this first pass?

@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions

Copy link
Copy Markdown

Colab 👈 Launch a Colab session on branch hytop_settings

@IAlibay
IAlibay self-requested a review October 22, 2026 14:08
@@ -0,0 +1,305 @@
{

@IAlibay IAlibay Oct 23, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. [nit] maybe we can put the line about "Choose and Configure a Protocol" inside one of those info boxes?
  2. [nit] How about a table for the parameters list?
  3. Thinking about thw iderr range of settings, maybe this should be "settings which impact sampling / performance", and then we could have "settings which affect the termodynamic state" and "settings that affect the model".

Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed all of these, for point 3 the main contents are now under the heading Sampling and performance settings with the idea of adding the other sections later, let me know what you think.

@@ -0,0 +1,305 @@
{

@IAlibay IAlibay Oct 23, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"..., you may also need to consider increasing the number of lambda windows see the next section for more information." This sentence probably needs breaking into two sentences.

"Consider increasing the simulation length after running the simulation when:" Might be good to mention here that they'd have to run a new simulation (i.e. we can't extend at this time).

"Increasing the simulation length is not always the best solution...." This sentence is vague and maybe a bit a confusing. Technicaly you can just run a very long simulation to improve overlap - it might be good to say something like "increasing the number of windows can sometimes be more useful in increasing overlap". I.e. a combination of both would help, but think about lambda windows first.


Reply via ReviewNB

@hannahbaumann hannahbaumann Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • "A large number of samples are..": Maybe briefly mention where the user could find this information.
  • Do you have a citation for the kbT cutoff?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IAlibay agreed, changed.

@hannahbaumann I think this might be best saved for the shared health check cookbook which could have a similar format and we can link to it there?

Added the citation for the kbT cutoff but maybe this would also be best put in the health check cookbook when describing what each diagnostic is showing?

@IAlibay IAlibay left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this quite simple format okay to start with?

I think there's a lot of things here that could be generalized. As a first pass this is ok, but in follow-up work, it might be good to make this more abstract and make it cover multiple protocols.

Are there any other settings we should add in this first pass?

I left a comment above. I think it would be good to cover the broad "themes" of settings we have. E.g. things that control your model, things that control the thermodynamics, things that control sampling/convergence.

@@ -0,0 +1,305 @@
{

@hannahbaumann hannahbaumann Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Would we want a separate cookbook for SepTop/ABFE settings or could it make sense to showcase some difference here (mostly thinking of the lambda schedule).
  • Should we also highlight and explain adaptive_settings in this cookbook or should that be done elsewhere? (maybe wrt what happens to the user updated settings after calling adaptive_settings, meaning it wouldn't be changed back to default settings); maybe also mention them in the section that talks about net charge changes?

Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried to include the SepTop and ABFE settings changes as well as there is a lot of overlap let me know what you think; maybe the format could be cleaned up as its quite verbose right now, but I wanted each section to have a consistent flow.

_adaptive_settings is an awkward one as its related and is probably worth introducing the beginning after inspecting the defaults but maybe we should consider making it public first?

@@ -0,0 +1,305 @@
{

@hannahbaumann hannahbaumann Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Equilibration length and protocol repeats are mentioned here, but not discussed below. Would it make sense to also add some more details for those (when one would want to change them, what the default is,..)?


Reply via ReviewNB

@@ -0,0 +1,305 @@
{

@hannahbaumann hannahbaumann Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it make sense to generally have a section/table somewhere that has all the simulations diagnostics listed and then the possible settings changes, as a summary/overview?


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that could be a good idea, the plan is to add a general health check page which would be related to this maybe there we should include examples of good and bad outputs of the simulation diagnostic and then link to the sections of this cookbook which control a setting which should influence that output?

@@ -0,0 +1,305 @@
{

@hannahbaumann hannahbaumann Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #14.    new_total_sim_time = n_repeats * n_windows * (protocol.settings.simulation_settings.production_length + protocol.settings.simulation_settings.equilibration_length)

Since the times are per leg, maybe we could call it *_sim_time_per_leg?


Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

@@ -0,0 +1,305 @@
{

@hannahbaumann hannahbaumann Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line #5.    # The number of replicas must be increased to match the number of lambda windows 

Maybe change this to must since it would raise an error

Reply via ReviewNB

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to must.

@@ -0,0 +1,834 @@
{

@jthorton jthorton Oct 26, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to advertise the private function, its helpful to validate the new schedule but its private and could change in future and break. Maybe this validation could be moved back onto the settings object so we can call model_validate ?


Reply via ReviewNB

Comment thread cookbook/exploring_rbfe_settings.ipynb
@@ -0,0 +1,830 @@
{

@jthorton jthorton Oct 26, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this break the flow of the notebook? Maybe it would be better to do it from the view of RBFE and have a section at the bottom showing how to change these values for the others grouped by protocol so users can click on that specific protocol and follow the steps for it?


Reply via ReviewNB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants