Skip to content
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

MinimizationReporter example #34

Merged
merged 4 commits into from
Jan 12, 2024
Merged

MinimizationReporter example #34

merged 4 commits into from
Jan 12, 2024

Conversation

sef43
Copy link
Contributor

@sef43 sef43 commented Nov 2, 2023

This adds an example for using the new MinimizationReporter in OpenMM 8.1: openmm/openmm#4207

  • It cannot be merged until after the 8.1 release

Copy link
Member

@peastman peastman left a comment

Choose a reason for hiding this comment

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

This is looking really good.

"\n",
"You can report the status of [`simulation.minimizeEnergy()`](http://docs.openmm.org/development/api-python/generated/openmm.app.simulation.Simulation.html?#openmm.app.simulation.Simulation.minimizeEnergy) using a [`MinimizerReporter`](http://docs.openmm.org/development/api-python/generated/openmm.openmm.MinimizationReporter.html).\n",
"\n",
"The syntax for doing this is somewhat different to typical OpenMM functionality. Read the [API documentation](http://docs.openmm.org/development/api-python/generated/openmm.openmm.MinimizationReporter.html) for more explanation.\n",
Copy link
Member

Choose a reason for hiding this comment

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

In what way is it different?

Comment on lines 85 to 91
"# Create an instance of our reporter\n",
"reporter = Reporter()\n",
"\n",
"# Perform local energy minimization \n",
"simulation.minimizeEnergy(reporter=reporter)\n",
"\n",
"# plot the minimization progress\n",
Copy link
Member

Choose a reason for hiding this comment

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

Just a personal preference, but I'd remove the comments. We don't need a separate comment for every line of code, especially when all they do is repeat what you just said immediately above: "We now create an instance of the reporter and minimize the system with the reporter attached." I'd show the two lines that do that without additional comments, then break to a text cell that says, "Now let's plot the energies that were recorded by the reporter," and then put the plotting code in its own cell.

"id": "2e7b659d",
"metadata": {},
"source": [
"We then create a `MinimizationReporter` class that will print the current energy to the screen every 100 steps and save the energies to an array which we can plot later"
Copy link
Member

Choose a reason for hiding this comment

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

That should be, "every 100 iterations," not, "every 100 steps." Does it even take 100 iterations to minimize? You probably want to print more often.

It would be good to give a little more explanation of what we're doing. Something like this: "A MinimizationReporter is called after every iteration of minimization. It is an abstract class. To use it, you define your own subclass that reports results in whatever way you want. The reporter shown here prints the current energy to the screen..."

"source": [
"## Reporting Minimization\n",
"\n",
"You can report the status of [`simulation.minimizeEnergy()`](http://docs.openmm.org/development/api-python/generated/openmm.app.simulation.Simulation.html?#openmm.app.simulation.Simulation.minimizeEnergy) using a [`MinimizerReporter`](http://docs.openmm.org/development/api-python/generated/openmm.openmm.MinimizationReporter.html).\n",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add another sentence saying why you would want to do that?

@sef43 sef43 marked this pull request as ready for review December 28, 2023 21:13
"source": [
"## Reporting Minimization\n",
"\n",
"You can report the status of [simulation.minimizeEnergy()](http://docs.openmm.org/latest/api-python/generated/openmm.app.simulation.Simulation.html?#openmm.app.simulation.Simulation.minimizeEnergy) using a [MinimizationReporter](http://docs.openmm.org/development/api-python/latest/openmm.openmm.MinimizationReporter.html). Note that `MinimizationReporter` was introduced in OpenMM 8.1.\n",
Copy link
Member

Choose a reason for hiding this comment

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

The URL for MinimizationReporter is broken. It should be http://docs.openmm.org/latest/api-python/generated/openmm.openmm.MinimizationReporter.html.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for spotting that!

@peastman
Copy link
Member

peastman commented Jan 2, 2024

Looks good. Is it ready to merge? I notice that none of the cells have been run so no output is shown.

@sef43
Copy link
Contributor Author

sef43 commented Jan 12, 2024

The cookbook notebooks all get run by the website building workflow, they are then rendered on the webpage with the output generated during the github worlfow.

@sef43 sef43 merged commit 25befe0 into openmm:main Jan 12, 2024
1 of 2 checks passed
@peastman
Copy link
Member

Thanks!

@peastman
Copy link
Member

It still isn't showing up on the website...

@sef43
Copy link
Contributor Author

sef43 commented Jan 13, 2024

Its on the /dev page: https://openmm.github.io/openmm-cookbook/dev/notebooks/cookbook/report_minimization.html
We need to make a release on github then the current main branch will be rendered on the latest stable webpage:
https://openmm.github.io/openmm-cookbook/latest/cookbook.html

@sef43 sef43 mentioned this pull request Jan 13, 2024
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.

2 participants