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

Add WaterCluster testsystem #322

Merged
merged 12 commits into from
Jan 19, 2018
Merged

Add WaterCluster testsystem #322

merged 12 commits into from
Jan 19, 2018

Conversation

maxentile
Copy link
Member

@maxentile maxentile commented Jan 17, 2018

Creates a few waters in a harmonic restraining potential.

Implementation draws on WaterBox and LennardJonesCluster.

This was suggested by @jchodera as a testsystem that would allow a smaller number of water molecules than possible in WaterBox.

water_mov

@jchodera
Copy link
Member

Looks like there are some travis failures:

======================================================================
ERROR: Failure: NameError (global name 'm' is not defined)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda/envs/test/lib/python2.7/site-packages/nose/loader.py", line 251, in generate
    for test in g():
  File "/home/travis/miniconda/envs/test/lib/python2.7/site-packages/openmmtools-0.13.5-py2.7.egg/openmmtools/tests/test_integrators_and_testsystems.py", line 88, in test_integrators_and_testsystems
    testsystem = getattr(testsystems, testsystem_name)()
  File "/home/travis/miniconda/envs/test/lib/python2.7/site-packages/openmmtools-0.13.5-py2.7.egg/openmmtools/testsystems.py", line 1778, in __init__
    new_pos = m.getPositions()
NameError: global name 'm' is not defined

@jchodera
Copy link
Member

I've checked in a fix for the typo.

@jchodera
Copy link
Member

For some reason, the test for is_quantity_close() is failing:

======================================================================
FAIL: Test is_quantity_close method.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda/envs/test/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/miniconda/envs/test/lib/python2.7/site-packages/openmmtools-0.13.5-py2.7.egg/openmmtools/tests/test_utils.py", line 82, in test_is_quantity_close
    assert is_quantity_close(quantity1, quantity2) is test_result
AssertionError

@maxentile
Copy link
Member Author

Ahh, thanks!

Looking at the is_quantity_close test...

@maxentile
Copy link
Member Author

On all 3 Travis builds, it looks like the test that checks whether 300.0 K is close to 300.000000004 K is failing.

However, running nosetests test_utils.py locally doesn't reproduce this problem -- I wonder if the behavior of numpy.isclose changed recently...

@maxentile
Copy link
Member Author

I wonder if the behavior of numpy.isclose changed recently...

I think that may be the culprit.

Conda-upgrading numpy: 1.12.1-py36_blas_openblas_200 conda-forge [blas_openblas] --> 1.12.1-py36_blas_openblas_201 conda-forge [blas_openblas]

Changes the behavior of

import numpy as np
value1 = 300
value2 = 300.00000004
np.isclose(value1, value2, rtol=1e-10, atol=0.0) # True before, False now
np.isclose(value2, value1, rtol=1e-10, atol=0.0) # True before, False now

Propose we loosen the test

@maxentile
Copy link
Member Author

Okay that didn't fix it-- the test is still failing on travis but passing locally...

Reverting previous commit...

Revert
7c6bbb9f017ff
029a2b8943c366739b02b41e785
Andrea had recently addressed this here!
00d2df1508000
f8961ba6026d58a2c710f630532
@@ -79,7 +79,8 @@ def test_is_quantity_close():
(1.01325*unit.bar, 1.01325000006*unit.bar, True),
(1.01325*unit.bar, 1.0132500006*unit.bar, False)]
for quantity1, quantity2, test_result in test_cases:
assert is_quantity_close(quantity1, quantity2) is test_result
msg = "Test failed: ({}, {}, {})".format(quantity1, quantity2, test_result)
assert is_quantity_close(quantity1, quantity2) == test_result, msg
Copy link
Member

Choose a reason for hiding this comment

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

NICE CATCH! That was driving me crazy.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was @andrrizzi ! He identified this issue in 00d2df1 and pointed me to the fix.

Addressing Code Climate "Issues to Fix"
@maxentile
Copy link
Member Author

Tests pass, should be good to merge.

@jchodera
Copy link
Member

Thanks! I'll add to the docs and changelog and then merge.

@maxentile
Copy link
Member Author

Oh! I forgot to do that -- no need for you to do that, I'll update the docs and changelog

@jchodera
Copy link
Member

jchodera commented Jan 19, 2018

We should really add pull request checklists. I forget all the time too.

I'd add the water box here and create a new changelog entry above here called "development snapshot".

Under both `Clusters and simple fluids` and `Water boxes`
@jchodera
Copy link
Member

Thanks!

@jchodera jchodera merged commit 0359f68 into master Jan 19, 2018
maxentile added a commit that referenced this pull request Jan 19, 2018
Suggested by John here:
#322 (comment)
8
@maxentile maxentile deleted the water-cluster branch January 19, 2018 20:57
@maxentile maxentile restored the water-cluster branch January 19, 2018 21:23
@jchodera jchodera deleted the water-cluster branch March 7, 2018 21:46
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