shirtsgroup / InterMol

Conversion tool for molecular simulations
MIT License
186 stars 53 forks source link

Simplify energy grouping accross simulators #308

Closed ctk3b closed 7 years ago

ctk3b commented 7 years ago

This is an attempt to provide more info in the energy output and streamline the process a bit across all of the platforms.

convert.py contains the master list of canonical names as before. Each simulator implements a dict called to_canonical that maps names native to that simulator to our canonical names. All the grouping and conversions are handled either by 1) the general canonicalize_energy_names or 2) by tweaking the values in the to_canonical dicts.

Additional, this fixes amber files not being correctly converted.

mrshirts commented 7 years ago

OK, let's make sure we don't get rid of all of the proper lumping of terms together in bins that are comparable. If that's preserved, we're all good.

ctk3b commented 7 years ago

I think everything looks good for GROMACS and LAMMPS. Working on getting the ParmEd amber parser to read in RB torsions right now and if that works I'll get the amber energy terms updated too.

ctk3b commented 7 years ago

Working with https://github.com/ParmEd/ParmEd/pull/837 this appears to work for AMBER. I'll add the charmm and desmond canonicalization tomorrow. Might need some help validating the desmond and charmm portions if you have some time tomorrow @mrshirts

mrshirts commented 7 years ago

I'll have a little time, so keep me posted on what you need me to test.

ctk3b commented 7 years ago

Ok I think this is ready to look at.

@mrshirts do the groupings in to_canonical for CHARMM and Desmond make sense?

ctk3b commented 7 years ago

@mrshirts when you find some time could you look over this?

There's still a dihedral energy mismatch for gromacs rb_torsions->amber but this PR now actually lets users run the conversion properly which wasn't working before.

mrshirts commented 7 years ago

Which files should I run to look at the results and see how they have changed?

ctk3b commented 7 years ago

This should work with any of the unit tests.

The logic here and here is what I'm least familiar with. The values in those dicts are the categories that the keys will get added to in the final output.

mrshirts commented 7 years ago

I think there are still some issues with lumping terms. For example, in a test gromacs -> desmond, we have:


            type     input(gromacs)  output (desmond)    diff (desmond)
           angle     31631.03693255    22880.93486112    -8750.10207143
            bond      6172.06789834    14922.14114872     8750.07325038
    urey_bradley         0.00000000        0.00000000        0.00000000

Which shouldn't happen -- the urey-bradley terms should probably be properly attributed to urey-bradley.

And there are a lot of other confusions. For example, looking at nonbonded (that aren't 14's), we have the entries:


        coulomb    -82909.22870679        0.00000000    82909.22870679
   disper. corr.      -124.07191053        0.00000000      124.07191053
      dispersive     -4332.05426045    -8723.56694736    -4391.51268691
   electrostatic         0.00000000        0.00000000        0.00000000
       nonbonded     22938.98696072   105848.21225976    82909.22529904
             vdw     -8723.55756088    -8723.56694736       -0.00938648

Clearly, electrostatic should either not be listed, or have other things in it. And the total nonbonded can't be off by 82909 kJ/mol when the bonded is only off by -375, with the total off by -98 kJ/mol.

mrshirts commented 7 years ago

So, I'm trying to think through the logic of what to fix first. I think before addressing the other issues, I will first do some editing to this one to clean up the grouping, then add some CHARMM tests to make sure they are being grouped correctly as well.

mrshirts commented 7 years ago

OK, should I submit a pull request to ctk3b:energies with proposed changes?

ctk3b commented 7 years ago

Yes that's probably easiest.

mrshirts commented 7 years ago

progress on CI checks? No worries if not yet, I just didn't want to merge until those are resolved.

ctk3b commented 7 years ago

I'm fixing the CI in #324

mrshirts commented 7 years ago

Hmm, still seems to be failing now.

ctk3b commented 7 years ago

We'll have to merge #324 first. If you're OK with those changes, I can take care of merging that

ctk3b commented 7 years ago

This should be good now too.

One of the mac stress tests timed out - I restarted it but we should probably stick smaller stress tests or skip the really big ones on CI and just use them to benchmark performance.

mrshirts commented 7 years ago

I'm fine with removing the stress tests. We should (medium term) be taking interesting things from the stress tests and making unit tests out of them anyway.