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

Bugfix in Nose-Hoover chain integrator #384

Merged
merged 3 commits into from Nov 8, 2018
Merged

Conversation

Olllom
Copy link
Contributor

@Olllom Olllom commented Nov 6, 2018

@andysim and I realized that constraints had not been taken into account when calculating the number of degrees of freedom in the NoseHooverChainVelocityVerletIntegrator.
As a result, the system temperature did not converge to the target value for systems with constraints.

I added a test to make sure that the target temperature is now matched.

The fix changes the API of the constructor: It requires the system to be passed as an argument (only in order to extract the correct number of constraints; the integrator then forgets about the system).

Cheers,
Andreas Krämer

Constraints were not taken into account when calculating
the number of degrees of freedom.
openmmtools/integrators.py Outdated Show resolved Hide resolved
@andrrizzi
Copy link
Contributor

Looks good to me! Could you fix the doctests and the regular tests that got broken with the API?

@andrrizzi
Copy link
Contributor

In particular, some tests in test_integrators_and_testsystems.py and many tests in test_integrators.py (search for integrator_class()) assume that you can instantiate an Integrator without arguments, so if we don't make system an optional argument we need to create an exception instantiating it in the test code.

@Olllom
Copy link
Contributor Author

Olllom commented Nov 7, 2018

@andysim Thanks for the catch.

@andrrizzi
Instead of adding an exception to all the tests, I made system=None a keyword argument of the constructor.
The integrator will fall back to the old behavior, which is correct for unconstrained systems. A severe warning will be printed, if the system is not specified.

  • This avoids changing all the tests in test_integrator.py
  • In contrast, an exception was added to test_integrator_and_testssystem.py - it actually makes of sense to test if the integrator plays nicely with all test systems.

Copy link
Contributor

@andrrizzi andrrizzi left a comment

Choose a reason for hiding this comment

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

Sounds good, thank you!

@andrrizzi andrrizzi merged commit a0c3f5a into choderalab:master Nov 8, 2018
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.

None yet

3 participants