Parmed conversions #334

daico007 · 2020-03-04T05:27:02Z

Something happens with my original Parmed conversions PR so I open a new one here. This PR adds to_parmed method, allowing the conversion from gmso.Topology to parmed.Structure. There is an argument to also convert types and parameters information (default to True).
Basically, I am mapping:

Topology -> Structure
SubTop -> Residue
Sites -> Atoms
*Type -> *Type
There are sanity check to make sure the Type's expression matched with Parmed default.
This PR is built upon @ahy3nz PR (beefing up the from_parmed method) Parse dihedral information, parameters, expressions from parmed structure #169, Consolidate parmed to topology types #172, .

…test to look for dihedrals now

…d pmd.Structure

…/topology into parmed_conversions

Cleaning up code, need to switch branch for docathon

…/topology into parmed_conversions

…zed), written up code to convert AtomType

Add all the helper function for this method, to_parmed is working locally, I will add unit test to make sure all the parameters are converted correctly.

…to parmed_conversions

codecov · 2020-03-04T05:30:16Z

Codecov Report

Merging #334 into master will decrease coverage by 0.43%.
The diff coverage is 89.53%.

@@            Coverage Diff             @@
##           master     #334      +/-   ##
==========================================
- Coverage   95.06%   94.63%   -0.44%     
==========================================
  Files          66       66              
  Lines        3873     4193     +320     
==========================================
+ Hits         3682     3968     +286     
- Misses        191      225      +34

Impacted Files	Coverage Δ
gmso/external/__init__.py	`100% <100%> (ø)`	⬆️
gmso/tests/base_test.py	`100% <100%> (ø)`	⬆️
gmso/tests/test_convert_parmed.py	`90.9% <88.13%> (-9.1%)`	⬇️
gmso/external/convert_parmed.py	`90.26% <89.32%> (-7.18%)`	⬇️
gmso/core/topology.py	`92.68% <0%> (+0.48%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update feee88f...8346081. Read the comment docs.

mattwthompson

Thanks for this! This is no small feat. I will let the linters and CI bots suggest their own changes, but I think the missing pieces here are just more tests, maybe a few along the lines of

A system with other elements than C & H
A system with multiple residues (i.e. two waters in one structure)
Round-trip coversions
Checking
I wonder if each of the _consolidate_core_type functions in from_parmed would be better placed in discrete internal functions, since each block of code produces a single output (the dict). I noticed that you have done this in the opposite direction to_parmed

mattwthompson · 2020-03-04T12:57:09Z

gmso/external/convert_parmed.py

+    # simple check if our pmd.Structure is fully parametrized
+    is_parametrized = True if isinstance(structure.atoms[0].atom_type,
+            pmd.AtomType) else False


Can this this be done more comprehensively? Looking at more atom types and some connection types. I believe pmd can operate with partially-parametrized systems. Unless the point here is to check if anything is parametrized at all

mattwthompson · 2020-03-04T12:59:36Z

gmso/external/convert_parmed.py

+                        'sigma': (atom_type.sigma * u.angstrom).in_units(u.nm),
+                        'epsilon': atom_type.epsilon * u.Unit('kcal / mol')},


Stuff like this is probably worth discussing in the context of #5 #315 - I don't think there's necessarily a downside to converting it into our internal units but it does seem like an unnecessary function call

mattwthompson · 2020-03-04T13:15:05Z

gmso/tests/test_convert_parmed.py

+        assert struc.bond_types == struc_from_top.bond_types
+        assert struc.angle_types == struc_from_top.angle_types
+        assert struc.dihedral_types == struc_from_top.dihedral_types
+        assert struc.rb_torsion_types == struc_from_top.rb_torsion_types


How are pmd's equality overrides done? Does this go through and check each attribute of the object against the other? My first guess in a round-trip conversion would be to compare the GMSO objects but this could be easier (and I don't see any reason to believe it tests different behavior, unless there was a reason to do two full loops)

For Parmed, two bond types are equal if their parameters are equal. Parmed Structure does not store atom_types, so I cannot compare that one

daico007 · 2020-03-04T15:26:12Z

I will try to work on breaking _consolidate_core_type into smaller chunks

Break up _consolidate_core_type into bite size chunks, Add more unit tests and pytest.fixture, Update documentation.

ahy3nz

Check your parmed_loop test since that gets skipped.

Watch the the use of default mutable arguments for Angle and Dihedral constructors

import mbuild as mb
import mbuild.recipes
import foyer
from gmso.external.convert_parmed import from_parmed, to_parmed

pentane = mb.recipes.Alkane(5)
pentane.name = "PENT"
box = mb.fill_box(pentane, n_compounds=3, box=[5,5,5])
struc = foyer.forcefields.load_OPLSAA().apply(box, residues="PENT")
print(struc.residues)
top = from_parmed(struc)
print(top.subtops)
new_struc = to_parmed(top)
print(new_struc.residues)

Subtops don't quite match the residues. FF params and bondtypes etc look pristine though

ahy3nz · 2020-03-04T23:57:26Z

gmso/external/convert_parmed.py

+    for dihedraltype in structure.dihedral_types:
+        dihedral_params = {
+                'k': (dihedraltype.phi_k * u.Unit('kcal / mol')),
+                'phi_eq': (dihedraltype.phase * u.degree),


Is there anything about the different interpretations of phi_eq that apply here?

Do you mean the different names that variable can have?

#31

phi_eq as compared to cis or trans I think. If we take phi_eq as trans, and parmed takes phi_eq as cis, then we're pi radians off

ahy3nz · 2020-03-04T23:57:53Z

gmso/external/convert_parmed.py

+    pmd_top_dihedraltypes = {}
+    for dihedraltype in structure.dihedral_types:
+        dihedral_params = {
+                'k': (dihedraltype.phi_k * u.Unit('kcal / mol')),


Is there a factor of 2 we need to worry about?

Do you know where I can look up parmed default equation for these Types? I try to look for these in their documentation but could not find anything helpful. Right now, i am basing this conversion (as in if we need a factor of 2) on your earlier version of from_parmed (from your other 2 PRs).

You kind of have to work backwards from how these parameters are stored in OpenMM or gromacs etc, and then figure out parmed digests those parameters to get an idea of what parmed's form is

I checked the Parmed gro writer and there is no factor of 2 for dihedral's k

Okay then I think the gmso equation for periodic torsions matches openmm/gromacs so you're good that no factor of 2 conversion is necessary

Fix residue-subtop mapping for both to_parmed and from_parmed. Add unit tests for residues and boxes conversion.

ahy3nz · 2020-03-10T23:50:32Z

Residue translation from the earlier example looks good now

mattwthompson

I think these changes are big enough we should try to merge soon, knowing that we'll be tinkering around the edges of ParmEd conversions in perpituity 🙂 (one that comes to mind is impropers, which we just added, but that is appropriate for another PR)

ahy3nz and others added 18 commits October 11, 2019 13:43

Parse periodic and rbtorsion dihedrals from parmed structure, update …

03a091e

…test to look for dihedrals now

Pass member types to connection types from parmed

92de85f

Make only one top.BondType per pmd.BondType, if we have a parametrize…

e6df80d

…d pmd.Structure

Merge commit 'refs/pull/169/head' of https://github.com/mattwthompson…

cb9bf1d

…/topology into parmed_conversions

update PR 169 to cope with recent changes

64a7f74

Add atoms and bonds matching

9e29396

Checkpoint

e514868

Cleaning up code, need to switch branch for docathon

Merge commit 'refs/pull/172/head' of https://github.com/mattwthompson…

6488287

…/topology into parmed_conversions

add template and to do list

a8fc268

typo

1bfea81

fix merge conflict

c1b14d0

working prototype converting to barebone pmd.Structure (un-parametrai…

d5ebc00

…zed), written up code to convert AtomType

add atomtype, bondtype, angletype conversion helper code

5950507

bug fix

9c9c09a

Merge remote-tracking branch 'upstream/master' into parmed_conversions

deaafad

Finalizing to_parmed function

99598bf

Add all the helper function for this method, to_parmed is working locally, I will add unit test to make sure all the parameters are converted correctly.

Add unit tests

5417cfb

Merge branch 'master' of https://github.com/mattwthompson/topology in…

3951c3a

…to parmed_conversions

daico007 added enhancement New feature or request ready for review labels Mar 4, 2020

mattwthompson reviewed Mar 4, 2020

View reviewed changes

Update convert_parmed, add test cases

cb1bbf1

Break up _consolidate_core_type into bite size chunks, Add more unit tests and pytest.fixture, Update documentation.

ahy3nz reviewed Mar 5, 2020

View reviewed changes

daico007 and others added 5 commits March 5, 2020 23:46

Fix res-suptop mapping, add more unit

be73ddc

Fix residue-subtop mapping for both to_parmed and from_parmed. Add unit tests for residues and boxes conversion.

double check unit and conversion factor

1c92f05

Merge branch 'master' into parmed_conversions

ea163db

add rb_torsions and titles check

97d1cd4

resolve merge conflicts

3586a2c

fix merge conflict

ef58149

add openbabel to requirement-test

8346081

mattwthompson approved these changes Mar 12, 2020

View reviewed changes

daico007 mentioned this pull request Mar 12, 2020

OpenMM conversion #350

Open

daico007 requested review from ahy3nz and justinGilmer March 13, 2020 15:59

ahy3nz approved these changes Mar 13, 2020

View reviewed changes

daico007 merged commit a452689 into mosdef-hub:master Mar 16, 2020

daico007 mentioned this pull request Mar 31, 2020

Conversion to ParmEd Structure #214

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parmed conversions #334

Parmed conversions #334

daico007 commented Mar 4, 2020

codecov bot commented Mar 4, 2020 •

edited

Loading

mattwthompson left a comment

mattwthompson Mar 4, 2020

mattwthompson Mar 4, 2020

mattwthompson Mar 4, 2020

daico007 Mar 4, 2020

daico007 commented Mar 4, 2020

ahy3nz left a comment

ahy3nz Mar 4, 2020

mattwthompson Mar 7, 2020

ahy3nz Mar 10, 2020

ahy3nz Mar 4, 2020

daico007 Mar 6, 2020

ahy3nz Mar 7, 2020

daico007 Mar 9, 2020

ahy3nz Mar 10, 2020

ahy3nz commented Mar 10, 2020 •

edited

Loading

mattwthompson left a comment

		'sigma': (atom_type.sigma * u.angstrom).in_units(u.nm),
		'epsilon': atom_type.epsilon * u.Unit('kcal / mol')},

Parmed conversions #334

Parmed conversions #334

Conversation

daico007 commented Mar 4, 2020

codecov bot commented Mar 4, 2020 • edited Loading

Codecov Report

mattwthompson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daico007 commented Mar 4, 2020

ahy3nz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahy3nz commented Mar 10, 2020 • edited Loading

mattwthompson left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 4, 2020 •

edited

Loading

ahy3nz commented Mar 10, 2020 •

edited

Loading