Skip to content

fix omt is None error in test suite#954

Merged
dwhswenson merged 1 commit intoopenpathsampling:masterfrom
sroet:fix_openmm_tools_test
Dec 25, 2020
Merged

fix omt is None error in test suite#954
dwhswenson merged 1 commit intoopenpathsampling:masterfrom
sroet:fix_openmm_tools_test

Conversation

@sroet
Copy link
Member

@sroet sroet commented Dec 24, 2020

If OpenMM is installed, but OpenMMTools isn't, the test TestFeatures.test_copy_with_replacement_openmm
fails with:

    def test_copy_with_replacement_openmm(self):
        if not paths.integration_tools.HAS_OPENMM:
            raise SkipTest
        # test an openmm snapshot
>       sys = omt.testsystems.AlanineDipeptideVacuum()
E       AttributeError: 'NoneType' object has no attribute 'testsystems'

This is due to the code-block at the start of that file:

try:                                                                            
    import openmmtools as omt                                                   
except ImportError:                                                             
    omt = None   

This alters the test such that it is also skipped if omt is None

Also cleans up some pep8 complains in that file (mostly unused imports)

@dwhswenson
Copy link
Member

dwhswenson commented Dec 25, 2020

Good catch; will merge after #955 so that the version number on the main branch is correct.

Our testing didn't catch this because we only test with no integrations or "all" integrations ("all" in quotes because some integrations, e.g., Gromacs, are only tested indirectly). This is because it is just too much of a pain in CI to test each integration separately. But it's possible that, as here, there's a problem when only part of a set of related integrations is installed.

However, as we switch to GitHub Actions, we might try adding scheduled workflows (maybe weekly?) to check various individual integrations. It could give us better coverage of user environments without adding burden to the CI for the normal development cycle.

Final note: We usually use OpenMMTools for its integrators. Beware of the built-in OpenMM integrators. They're not reversible, and that's not only theoretically wrong for path sampling, but it actually leads to real problems. See the extensive discussion in #558.

@dwhswenson dwhswenson merged commit 3a681c1 into openpathsampling:master Dec 25, 2020
@sroet sroet deleted the fix_openmm_tools_test branch December 28, 2020 11:41
@dwhswenson dwhswenson mentioned this pull request Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants