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 more informative info to system_checker #228

Merged
merged 7 commits into from
Jul 12, 2016
Merged

Add more informative info to system_checker #228

merged 7 commits into from
Jul 12, 2016

Conversation

davidlmobley
Copy link
Collaborator

Adding more informative output information to system_checker for comparing details of OpenMM systems, as per #227 .

So far I just added additional info for periodic torsional forces. I'll be adding this in the order in which I need it.

@davidlmobley davidlmobley self-assigned this Jul 4, 2016
@jchodera
Copy link
Member

jchodera commented Jul 4, 2016

The first failure is happening because there is no minimization prior to simulation here.

Looks like the second failure is the same reason.

@davidlmobley
Copy link
Collaborator Author

Yes, as in #223 . Apparently something changed in the not-too-distant past in OpenMM which made it so simulations which would formerly simulate without being minimized first now cannot. I've run into this in many many places here and elsewhere, where there was old code (from Kyle I think) which would previously run but now does not.

My failure to reproduce in #223 was due to the fact that I still had an older OpenMM on my computer so it worked fine for me.

I suppose I should fix this as part of this PR, though it's really an unrelated issue.

@jchodera
Copy link
Member

jchodera commented Jul 4, 2016

Yes, as in #223 . Apparently something changed in the not-too-distant past in OpenMM which made it so simulations which would formerly simulate without being minimized first now cannot.

You sure it isn't packmol?

@jchodera
Copy link
Member

jchodera commented Jul 4, 2016

I suppose I should fix this as part of this PR, though it's really an unrelated issue.

Would be great if you could! You can add

simulation.minimizeEnergy()

@davidlmobley
Copy link
Collaborator Author

Yes, definitely not packmol. It's unchanged since the tests were working. Changes on OpenMM side.

@jchodera
Copy link
Member

jchodera commented Jul 4, 2016

Wow, looks like the initial energy is actually nan or something:

======================================================================
ERROR: test_packmol.test_packmol_simulation_ternary
----------------------------------------------------------------------
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/openmoltools-0.7.1.dev0-py2.7.egg/openmoltools/tests/test_packmol.py", line 51, in test_packmol_simulation_ternary
    simulation.minimizeEnergy()
  File "/home/travis/miniconda/envs/_test/lib/python2.7/site-packages/simtk/openmm/app/simulation.py", line 128, in minimizeEnergy
    mm.LocalEnergyMinimizer.minimize(self.context, tolerance, maxIterations)
  File "/home/travis/miniconda/envs/_test/lib/python2.7/site-packages/simtk/openmm/openmm.py", line 11057, in minimize
    return _openmm.LocalEnergyMinimizer_minimize(context, tolerance, maxIterations)
Exception: Particle coordinate is nan

I'll look into this separately if you want to create an issue and tag me.

You can merge this PR, though!

@davidlmobley
Copy link
Collaborator Author

@jchodera - yeah, I just saw the same thing. Really weird.

I was actually going to leave this issue open (hence the [WIP]) until I got a chance to make the debug info more clear, unless there is some reason to merge now.

@jchodera
Copy link
Member

jchodera commented Jul 4, 2016

No pressing need!

@davidlmobley
Copy link
Collaborator Author

@jchodera :

I can clear up some units, etc., info in these if you can answer this for me: If I have an OpenMM simulation and I loop over forces, like so:

for force in simulation0.system.GetForces():
       if type(force) == mm.PeriodicTorsionForce:
               force0 = force
               i0, i1, i2, i3, per, phase, k0 = force0.getTorsionParameters(k)
               phase_unit, k0_unit = phase.unit, k0.unit
               phase, k0 = phase / phase_unit, k0 / k0_unit
               print( per, phase, k0 )

or something along those lines (summarizing some of Kyle's code from system_checker), then what units do phase, and k0 have? k0 is probably the one where there is the most question.

I find the whole idea of dividing by units very confusing. It makes sense to me when it's in the context of a print statement, as you're just asking it to print out the value in the corresponding units. But I struggle with the above because the question is what units it was in at the end...

Also, if I have k0_unit, say, how do I get it to give me a string describing the name? I've tried k0_unit.name and this yields an exception.

The relevant code is basically here: https://github.com/choderalab/openmoltools/pull/228/files#diff-75c255d30df61e86edf538dbaf4b6f58R460

@jchodera
Copy link
Member

jchodera commented Jul 5, 2016

If you have a quantity q, you don't want to compute q/q.unit and then guess the units since this defeats the point of using units.

Instead, ask for things in the units you want. If you want kcal/mol, you can get a dimensionless number with q/unit.kilocalories_per_mole.

The periodicity is unitless, and k has units compatible with kilocalories_per_mole.

@davidlmobley
Copy link
Collaborator Author

John,

On Mon, Jul 4, 2016 at 5:49 PM, John Chodera [email protected]
wrote:

If you have a quantity q, you don't want to compute q/q.unit and then
guess the units since this defeats the point of using units.

Indeed! I was sort of horrified to see that in the code. Like I said, it
resulted in me being very confused. :)

Instead, ask for things in the units you want. If you want kcal/mol, you
can get a dimensionless number with q/unit.kilocalories_per_mole.

OK, excellent. I'll go in and just replace everywhere whoever (Kyle?)
divided values by the units of the value with something more sensible like
this. :)

The periodicity is unitless, and k has units compatible with
kilocalories_per_mole.

Right, and phase should be compatible with radians... :)

DM


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#228 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AGzUYVb0tot4taku-e9DxcB76-ObT9SKks5qSaobgaJpZM4JEntW
.

David Mobley
[email protected]
949-385-2436

@jchodera
Copy link
Member

jchodera commented Jul 5, 2016

Right, and phase should be compatible with radians... :)

No! The barrier heights k don't get multiplied or divided by radians, since the functional form is like k*cos((2*pi/periodicity)*(phi - phase)).

@davidlmobley
Copy link
Collaborator Author

Ah, I see what you mean. Odd. Weird to use notation of an angle (i.e. phi) if it's not really an angle.

@jchodera
Copy link
Member

jchodera commented Jul 5, 2016 via email

@davidlmobley davidlmobley changed the title [WIP] Add more informative info to system_checker Add more informative info to system_checker Jul 12, 2016
@davidlmobley
Copy link
Collaborator Author

Merging this, as I've fixed a bunch of units and debugging info, and I haven't broken any of the tests which weren't already broken.

@davidlmobley davidlmobley merged commit 9459c86 into master Jul 12, 2016
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