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

Fix sampler bugs, cut down NCMC testing time, add alkanes test system #109

Merged
merged 35 commits into from
Feb 14, 2016

Conversation

jchodera
Copy link
Member

This cuts down the NCMC testing time to speed up travis.

@jchodera
Copy link
Member Author

I've also added an alkanes testsystem.

@jchodera jchodera changed the title Cut down NCMC testing time Cut down NCMC testing time; add alkanes test system Feb 12, 2016
@juliebehr
Copy link

Among the errors (this happens a few times and was also happening in #92):
Failure: ImportError (cannot import name 'thermodynamics') ... ERROR
ERROR: Failure: ImportError (No module named 'thermodynamics')

The only other things that seem to be wrong have to do with TopologyProposals taking different inputs and having different attributes now, which will be fixed in #93 I think

@jchodera
Copy link
Member Author

Among the errors (this happens a few times and was also happening in #92):

Failure: ImportError (cannot import name 'thermodynamics') ... ERROR
ERROR: Failure: ImportError (No module named 'thermodynamics')

I had totally missed that! Hopefully fixed now.

The only other things that seem to be wrong have to do with TopologyProposals taking different inputs and having different attributes now, which will be fixed in #93 I think

Is that something I can fix in this PR?

@jchodera
Copy link
Member Author

The other failures seem to occur in geometry.py:

======================================================================
FAIL: Doctest: perses.samplers.samplers.SAMSSampler
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/choderaj/miniconda/lib/python2.7/doctest.py", line 2226, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for perses.samplers.samplers.SAMSSampler
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 627, in SAMSSampler

----------------------------------------------------------------------
File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 669, in perses.samplers.samplers.SAMSSampler
Failed example:
    sams_sampler.run()
Exception raised:
    Traceback (most recent call last):
      File "/Users/choderaj/miniconda/lib/python2.7/doctest.py", line 1315, in __run
        compileflags, 1) in test.globs
      File "<doctest perses.samplers.samplers.SAMSSampler[12]>", line 1, in <module>
        sams_sampler.run()
      File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 742, in run
        self.update()
      File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 728, in update
        self.update_sampler()
      File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 706, in update_sampler
        self.sampler.update()
      File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 608, in update
        self.update_sampler()
      File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 571, in update_sampler
        geometry_new_positions, geometry_logp  = self.geometry_engine.propose(topology_proposal)
      File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/rjmc/geometry.py", line 91, in propose
        beta = top_proposal.beta
    AttributeError: 'TopologyProposal' object has no attribute 'beta'

@pgrinaway: Any chance we could get in a quick fix for that?

@pgrinaway
Copy link
Member

Do you mind if I correct a GeometryEngine API call in the ExpandedEnsembleSampler in the next quick PR, or would that mess your PR up?

@pgrinaway
Copy link
Member

Ok, there are a bunch of other errors related to Python 3-incompatibility (import stuff mostly). Should I address those too?

@jchodera
Copy link
Member Author

Ok, there are a bunch of other errors related to Python 3-incompatibility (import stuff mostly). Should I address those too?

I will just comment out python 3.x for now. We can address the py3 stuff in a separate PR.

@jchodera
Copy link
Member Author

Do you mind if I correct a GeometryEngine API call in the ExpandedEnsembleSampler in the next quick PR, or would that mess your PR up?

Go for it!

@jchodera
Copy link
Member Author

I've commented out the py3 tests

@jchodera
Copy link
Member Author

Thanks, guys! I was able to fix a ton of bugs now that I was able to run all the tests.

@jchodera jchodera changed the title Cut down NCMC testing time; add alkanes test system [WIP] Fix sampler bugs, cut down NCMC testing time, add alkanes test system Feb 12, 2016
@jchodera
Copy link
Member Author

Should be ready to merge once travis concurs all test failures are gone.

@jchodera
Copy link
Member Author

OK, this seems to have been a syntax issue! I've sorted that, and am now running into geometry proposal problems:

======================================================================
ERROR: Testing expanded ensemble sampler with alanine dipeptide 'implicit'
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/choderaj/miniconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 674, in run
    self.update()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 661, in update
    self.update_sampler()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 623, in update_sampler
    geometry_new_positions, geometry_logp  = self.geometry_engine.propose(topology_proposal, ncmc_old_positions, self.sampler.thermodynamic_state.beta)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/rjmc/geometry.py", line 106, in propose
    new_positions[atom.idx] = current_positions[old_index]
  File "/Users/choderaj/miniconda/lib/python2.7/site-packages/simtk/unit/quantity.py", line 733, in __getitem__
    assert not is_quantity(self._value[key])
IndexError: index 25 is out of bounds for axis 0 with size 22
-------------------- >> begin captured logging << --------------------

@jchodera
Copy link
Member Author

I'm seeing issues with the PointMutationEngine generated TopologyProposal where the new_to_old_atom_map is nonsensical:

Testing SAMS sampler with alanine dipeptide 'vacuum' ... old system has 22 atoms; new system has 32 atoms
{0: 0, 1: 1, 2: 2, 3: 3, 4: 4, 5: 5, 6: 6, 7: 7, 8: 8, 9: 9, 10: 10, 11: 11, 12: 12, 13: 13, 14: 14, 15: 15, 16: 16, 17: 17, 18: 18, 19: 19, 20: 20, 21: 21, 22: 22, 23: 23, 24: 24, 25: 25, 26: 26, 27: 27, 28: 28, 29: 29, 30: 30, 31: 31}

Here, the old system only has 22 atoms, but old atom indices 20 and above are referenced in the atom mapping.

I'll insert some more assertion tests that catch exactly when this happens.

@jchodera
Copy link
Member Author

OK, I've tried to add some tests and can't identify any issues. Hrm....

@jchodera
Copy link
Member Author

I've added an assertion into PointMutationEngine.propose that ensures the new_to_old_atom_map is sensible:
https://github.com/choderalab/perses/blob/samplers/perses/rjmc/topology_proposal.py#L308-L318

This seems to fail for some mutations:

======================================================================
ERROR: Testing SAMS sampler with alanine dipeptide 'vacuum'
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/choderaj/miniconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 686, in run
    self.update()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 673, in update
    self.update_sampler()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 597, in update_sampler
    topology_proposal = self.proposal_engine.propose(system, topology)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/rjmc/topology_proposal.py", line 314, in propose
    raise Exception(msg)
Exception: Some old atoms in TopologyProposal.new_to_old_atom_map are not in span of old atoms (1..22):
{0: 0, 1: 1, 2: 2, 3: 3, 4: 4, 5: 5, 6: 6, 7: 7, 8: 8, 9: 9, 10: 10, 11: 11, 12: 12, 13: 13, 14: 14, 15: 15, 16: 17, 17: 21, 26: 25, 27: 26, 28: 27, 29: 28, 30: 29, 31: 30}

Still trying to track this down.

@jchodera
Copy link
Member Author

OK, the bug was that some of the PointMutationEngine.propose() code was using current_topology (which I think was mutable) while other parts of the code were using a deep copy old_topology. The whole thing now uses the deep copy (old_topology), and those topology proposal errors are gone.

The remaining errors are NCMC issues!

@jchodera
Copy link
Member Author

There appear to be some geometry engine issues too:


======================================================================
ERROR: Testing MultiTargetDesign sampler with alanine dipeptide mutation transfer free energy from vacuum -> implicit
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/choderaj/miniconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 934, in run
    self.update()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 919, in update
    self.update_samplers()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 889, in update_samplers
    sampler.update()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 811, in update
    self.update_sampler()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 785, in update_sampler
    self.sampler.update()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 673, in update
    self.update_sampler()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 635, in update_sampler
    geometry_new_positions, geometry_logp  = self.geometry_engine.propose(topology_proposal, ncmc_old_positions, self.sampler.thermodynamic_state.beta)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/rjmc/geometry.py", line 112, in propose
    torsion, logp_choice = self._choose_torsion(atoms_with_positions, atom)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/rjmc/geometry.py", line 546, in _choose_torsion
    torsion_idx = np.random.randint(0, len(eligible_torsions))
  File "mtrand.pyx", line 945, in mtrand.RandomState.randint (numpy/random/mtrand/mtrand.c:10732)
ValueError: low >= high

@jchodera
Copy link
Member Author

The NCMC failures appear to be due to the energy becoming NaN when the alchemical lambda values are set to 0:

======================================================================
ERROR: Testing expanded ensemble sampler with alanine dipeptide 'implicit'
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/choderaj/miniconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 692, in run
    self.update()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 679, in update
    self.update_sampler()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 635, in update_sampler
    [ncmc_old_positions, ncmc_elimination_logp, potential_delete] = self.ncmc_engine.integrate(topology_proposal, positions, direction='delete')
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/annihilation/ncmc_switching.py", line 327, in integrate
    raise NaNException("Initial potential of 'insert' operation is NaN (unmodified potential was %.3f kT, alchemical potential was %.3f kT before changing lambda)" % (unmodified_potential, alchemical_potential))
NaNException: Initial potential of 'insert' operation is NaN (unmodified potential was -35.407 kT, alchemical potential was -35.407 kT before changing lambda)

@jchodera
Copy link
Member Author

Whoops, looks like I was setting a bunch of other random non-alchemical context parameters to zero, like

solventDielectric 78.3
soluteDielectric 1.0
offset 0.009

@jchodera
Copy link
Member Author

OK, I'm down to just geometry engine errors:

======================================================================
ERROR: Testing expanded ensemble sampler with alanine dipeptide 'vacuum'
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/choderaj/miniconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 692, in run
    self.update()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 679, in update
    self.update_sampler()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 641, in update_sampler
    geometry_new_positions, geometry_logp  = self.geometry_engine.propose(topology_proposal, ncmc_old_positions, self.sampler.thermodynamic_state.beta)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/rjmc/geometry.py", line 112, in propose
    torsion, logp_choice = self._choose_torsion(atoms_with_positions, atom)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/rjmc/geometry.py", line 546, in _choose_torsion
    torsion_idx = np.random.randint(0, len(eligible_torsions))
  File "mtrand.pyx", line 945, in mtrand.RandomState.randint (numpy/random/mtrand/mtrand.c:10732)
ValueError: low >= high

…ck of available torsions.

Fix bug in test_elimination.
@jchodera
Copy link
Member Author

OK, it seems like this failure occurs only for ring-containing atoms:

======================================================================
ERROR: Testing MultiTargetDesign sampler with alkanes mutation transfer free energy from vacuum -> implicit
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/choderaj/miniconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 940, in run
    self.update()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 925, in update
    self.update_samplers()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 895, in update_samplers
    sampler.update()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 817, in update
    self.update_sampler()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 791, in update_sampler
    self.sampler.update()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 679, in update
    self.update_sampler()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 641, in update_sampler
    geometry_new_positions, geometry_logp  = self.geometry_engine.propose(topology_proposal, ncmc_old_positions, self.sampler.thermodynamic_state.beta)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/rjmc/geometry.py", line 112, in propose
    torsion, logp_choice = self._choose_torsion(atoms_with_positions, atom)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/rjmc/geometry.py", line 547, in _choose_torsion
    raise Exception("No eligible torsions found for placing atom %s." % str(atom_for_proposal))
Exception: No eligible torsions found for placing atom <Atom CD1 [16]; In PHE 1>.

It could be that only impropers torsions are defined for this atom.

@jchodera
Copy link
Member Author

I'm running into a few NCMC-based NaNs that I'll debug separately, but it looks like the only other major error is an underflow in the acceptance criteria:

======================================================================
ERROR: Testing MultiTargetDesign sampler with alanine dipeptide mutation transfer free energy from vacuum -> implicit
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/choderaj/miniconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 940, in run
    self.update()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 925, in update
    self.update_samplers()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 895, in update_samplers
    sampler.update()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 817, in update
    self.update_sampler()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 791, in update_sampler
    self.sampler.update()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 679, in update
    self.update_sampler()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 659, in update_sampler
    accept = ((logp_accept>=0.0) or (np.random.uniform() < np.exp(logp_accept)))
FloatingPointError: underflow encountered in exp

@jchodera
Copy link
Member Author

Whoops, spoke too soon. There's another issue now that I've fixed the alkanes test case:

======================================================================
ERROR: Testing expanded ensemble sampler with alkanes 'explicit'
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/choderaj/miniconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 697, in run
    self.update()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 684, in update
    self.update_sampler()
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/samplers/samplers.py", line 603, in update_sampler
    topology_proposal = self.proposal_engine.propose(system, topology)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/rjmc/topology_proposal.py", line 794, in propose
    smiles_new, _ = self._topology_to_smiles(new_topology)
  File "/Users/choderaj/github/choderalab/perses/perses.choderalab/perses/rjmc/topology_proposal.py", line 835, in _topology_to_smiles
    raise ValueError("More than one residue with the same name!")
ValueError: More than one residue with the same name!

@jchodera
Copy link
Member Author

Looks like resolving this last error is pending changes to SmallMoleculeSetProposalEngine to allow the user to specify the residue name of the molecule to be modified in the constructor instead of the receptor topology, so I"ll punt on that.

The other issues I need to address are

  • sporadic NCMC failures
  • underflow in evaluating acceptance probability

I'll merge this now and address those separately.

@jchodera jchodera changed the title [WIP] Fix sampler bugs, cut down NCMC testing time, add alkanes test system Fix sampler bugs, cut down NCMC testing time, add alkanes test system Feb 14, 2016
jchodera added a commit that referenced this pull request Feb 14, 2016
Fix sampler bugs, cut down NCMC testing time, add alkanes test system
@jchodera jchodera merged commit 7570e53 into master Feb 14, 2016
@juliebehr
Copy link

I'm very confused about the switch between old_topology and current_topology solving any problems -- previously the entire code depended on current_topology, so how does switching to the deep copy change anything? I specifically created the deep copy to not be altered through the course of propose so that when it is input to TopologyProposal I know it remains unchanged.

I would at least like to swap the names, if this is truly fixing the bug, but it would be helpful to understand what has happened here because in the future I would like to eliminate the Modeller altogether (it isn't being used, the topology it contains is altered directly, and it makes even less sense now that propose does not take positions) and add and delete atoms directly in current_topology.

@jchodera
Copy link
Member Author

I don't know the exact cause, but I think that current_topology was being modified in some unexpected way. Since old_topology was a deep copy of this, changes to old_topology do not propagate back into other parts of the code unexpectedly, but changes to current_topology do, causing weird interactions.

Feel free to do some refactoring in a PR if you want! I think we have pretty good test coverage right now, though we desperately need to fix the SmallMoleculeSetProposalEngine bugs (#110) that are causing huge cascades of failures.

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