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

[DNM] added openmm 8 beta to test matrix + fix bug with test #628

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mikemhenry
Copy link
Contributor

@mikemhenry mikemhenry commented Nov 1, 2022

Description

This PR adds openmm 8 to the testing matrix

Todos

  • Implement feature / fix bug
  • Add tests
  • Update documentation as needed
  • Update changelog to summarize changes in behavior, enhancements, and bugfixes implemented in this PR

Status

  • Ready to go

Changelog message

Added openmm 8 beta to testing matrix

Resolves #626

@mikemhenry
Copy link
Contributor Author

Interesting, getting the failure on this test:
Error: 0.00% test_utils.TestRestorableOpenMMObject.test_multiple_object_context_creation: 0.0012s

The output is not that helpful, will have to check this locally:

FAILED (errors=1)
        Friedrichs MS, Eastman P, Vaidyanathan V, Houston M, LeGrand S, Beberg AL, Ensign DL, Bruns CM, and Pande VS. Accelerating molecular dynamic simulations on graphics processing unit. J. Comput. Chem. 30:864, 2009. DOI: 10.1002/jcc.21209
        Eastman P and Pande VS. OpenMM: A hardware-independent framework for molecular simulations. Comput. Sci. Eng. 12:34, 2010. DOI: 10.1109/MCSE.2010.27
        Eastman P and Pande VS. Efficient nonbonded interactions for molecular dynamics on a graphics processing unit. J. Comput. Chem. 31:1268, 2010. DOI: 10.1002/jcc.21413
        Eastman P and Pande VS. Constant constraint matrix approximation: A robust, parallelizable constraint method for molecular simulations. J. Chem. Theor. Comput. 6:434, 2010. DOI: 10.1021/ct900463w
        Chodera JD and Shirts MR. Replica exchange and expanded ensemble simulations as Gibbs multistate: Simple improvements for enhanced mixing. J. Chem. Phys., 135:194110, 2011. DOI:10.1063/1.3660669
        
Testing Cython mixing code with uniform zero energies
################################################################
Testing Sampler: MultiStateSampler  -- States: 3  -- Samplers: 5
################################################################
################################################################
Testing Sampler: MultiStateSampler  -- States: 5  -- Samplers: 3
################################################################
##########################################################
Testing Sampler: SAMSSampler  -- States: 5  -- Samplers: 2
##########################################################
#######################################################################
Testing Sampler: ParallelTemperingSampler  -- States: 3  -- Samplers: 3
#######################################################################
#####################################################################
Testing Sampler: ReplicaExchangeSampler  -- States: 3  -- Samplers: 3
#####################################################################
##########################################################
Testing Sampler: SAMSSampler  -- States: 5  -- Samplers: 1
##########################################################
0 1
0 2
0 3
0 4
1 2
1 3
1 4
2 3
2 4
3 4

@mikemhenry
Copy link
Contributor Author

pytest gives me a better error:

FAILED openmmtools/tests/test_utils.py::TestRestorableOpenMMObject::test_multiple_object_context_creation - openmm.OpenMMException: Two Forces define different default values for the parameter '_restorable_force__class_hash'

@mikemhenry
Copy link
Contributor Author

This is the test

    def test_multiple_object_context_creation(self):
        """Test that it is possible to create contexts with multiple restorable objects."""
        system = openmm.System()
        for i in range(4):
            system.addParticle(1.0*unit.atom_mass_units)
        system.addForce(copy.deepcopy(self.dummy_force))
        system.addForce(copy.deepcopy(self.dummier_force))
>       context = openmm.Context(system, copy.deepcopy(self.dummy_integrator))

@mikemhenry
Copy link
Contributor Author

mikemhenry commented Nov 1, 2022

@mikemhenry
Copy link
Contributor Author

Added in this PR openmm/openmm#3769

@mikemhenry
Copy link
Contributor Author

So I think the issue is more with the test than a behavior change in openmm. My theory is that dummy_force and dummier_force both have _restorable_force__class_hash as a global param, but use two different values, which then throws that error. But I'm not sure if this is a problem in production, or just an artifact of the way we wrote this test.

@mikemhenry
Copy link
Contributor Author

@ijpulidos Do you mind taking a peak at this?

Comment on lines 63 to 68
- name: Install OpenMM nightly
shell: bash -l {0}
if: matrix.cfg.openmm == 'beta'
run: |
conda install -c conda-forge/label/openmm_rc -c conda-forge openmm

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there nightlies pushed to conda-forge? Or only betas/RCs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattwthompson So the "nightly" builds we do for openmm live at omnia-dev, but it doesn't use the conda-build infrastructure so it is kind of problematic. What I did was create a new label conda-forge/label/openmm_dev where I create new builds on demand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I did make a mistake on the name of this step!

Copy link
Contributor

@ijpulidos ijpulidos left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@mikemhenry mikemhenry changed the title added openmm 8 beta to test matrix {DNM] added openmm 8 beta to test matrix Nov 4, 2022
@mikemhenry mikemhenry changed the title {DNM] added openmm 8 beta to test matrix [DNM] added openmm 8 beta to test matrix Nov 4, 2022
@mikemhenry
Copy link
Contributor Author

I will use this PR to fix the one bug we seem to have with a test

@mikemhenry mikemhenry changed the title [DNM] added openmm 8 beta to test matrix [DNM] added openmm 8 beta to test matrix + fix bug with test Nov 4, 2022
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.

Add OpenMM 8 beta branch
3 participants