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

Minor bugs/Implementation improvements #772

merged 6 commits into from Jun 10, 2018


None yet
1 participant
Copy link

commented May 10, 2018

This PR fixes a few minor (and long-standing) bugs that would only cause problems at edge cases, but which became apparent while finishing up the OPS papers.

Bugs fixed:

  • Volumes should only include bounds on one side
  • Corrected implementation of TISEnsemble

Other stuff:

  • Update the minus ensemble to a better implementation
  • Clarify where we do overshoot trimming at a few points (still needs improved implementation)

Volumes should only include bounds on one side

Previously, CVDefinedVolumes were inclusive on both sides; i.e., they returned lambda_min <= cv(x) <= lambda_max. This meant that a single point could still be assigned to two ostensibly non-overlapping volumes.

It also meant that there was an inconsistency, thanks to the range logic factories. Consider the volume A from (-1, 0) and X (-1, 1). Previously, A(0) == True, because0 is the upper bound. Using range logic, the volume (X - A) covers (0, 1). Therefore (X - A)(0) == True, because 0 is the lower bound. But X - A means X & ~A; ~A(0) should be False, so (X & ~A)(0) should be False.

I fixed this by defining CVs such that they give lambda_min <= cv(x) < lambda_max. This fixes the above example because A(0) == False, and ~A(0) == True. This involves a lot of changes of the tests, because I frequently used the test case that we hit the border. Note that this is not likely to be a bug that has caused any trouble, since it only matters if you land exactly on the border and if you're using combinations that use range logic and if you're using a logical not somewhere.

I came across this problem when I was cleaning up the implementation of the MinusInterfaceEnsemble to make it look more like the description in the OPS 2 paper (which is better than the original implementation).

Corrected implementation of TISEnsemble

The previous implementation of the TISEnsemble put the check for a frame that crosses the interface in the middle subensemble. This meant that if the only frame that crossed the interface was the last frame (ending in the other state) we wouldnt catch it, and reject the trajectory. This is a small bug that only matters if the interface is very close to state B. In addition, it's worth noting that several mathematical descriptions of the TIS ensemble in the literature make the same mistake (using a < instead of a <=), so this is something that really requires careful attention.

dwhswenson added some commits May 9, 2018

@dwhswenson dwhswenson added the bugfix label May 10, 2018

@dwhswenson dwhswenson changed the title [WIP] Minor bugs/Implementation improvements Minor bugs/Implementation improvements May 11, 2018

@dwhswenson dwhswenson requested a review from jhprinz May 11, 2018


This comment has been minimized.

Copy link
Member Author

commented May 11, 2018

Ready for review/merge. If not reviewed by 20 May, I'll assume it is okay to merge.

@dwhswenson dwhswenson merged commit bb7b266 into openpathsampling:master Jun 10, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
coverage/coveralls Coverage increased (+0.01%) to 77.787%

@dwhswenson dwhswenson deleted the dwhswenson:ops_2_paper_updates branch Jun 10, 2018

@dwhswenson dwhswenson referenced this pull request Jun 20, 2018


Bump version to 0.9.5 #784

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.