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 HOOMD Supports #696

Merged
merged 41 commits into from
Apr 24, 2023
Merged

Add HOOMD Supports #696

merged 41 commits into from
Apr 24, 2023

Conversation

daico007
Copy link
Member

@daico007 daico007 commented Oct 5, 2022

[WIP] Start adding supports for HOOMD. Goal is to convert a typed GMSO Topology to a HOOMD state (gsd snapshot) and all the required forces.

Co Quach added 2 commits October 5, 2022 02:27
carried over code from gsd writer,
instead of saving the gsd file, just return the snapshot object
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 11, 2022

This pull request introduces 3 alerts when merging 49f62ff into 691371c - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Except block handles 'BaseException'

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 11, 2022

This pull request introduces 3 alerts when merging 1486fd0 into bc557dd - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Except block handles 'BaseException'

@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Patch coverage: 90.78% and project coverage change: -0.27 ⚠️

Comparison is base (c735cc3) 92.27% compared to head (bc51366) 92.01%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #696      +/-   ##
==========================================
- Coverage   92.27%   92.01%   -0.27%     
==========================================
  Files          65       66       +1     
  Lines        5890     6336     +446     
==========================================
+ Hits         5435     5830     +395     
- Misses        455      506      +51     
Impacted Files Coverage Δ
gmso/utils/sorting.py 86.27% <84.78%> (-13.73%) ⬇️
gmso/external/convert_hoomd.py 91.03% <91.03%> (ø)
gmso/external/__init__.py 100.00% <100.00%> (ø)
gmso/external/convert_mbuild.py 94.73% <100.00%> (+0.03%) ⬆️
gmso/formats/gsd.py 100.00% <100.00%> (+1.38%) ⬆️
gmso/formats/mcf.py 54.16% <100.00%> (ø)
gmso/formats/top.py 88.42% <100.00%> (ø)
gmso/utils/compatibility.py 96.66% <100.00%> (+0.11%) ⬆️
gmso/utils/geometry.py 88.88% <100.00%> (ø)
gmso/utils/io.py 96.82% <100.00%> (+0.27%) ⬆️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 16, 2022

This pull request introduces 3 alerts when merging b682255 into bc557dd - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Except block handles 'BaseException'

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 17, 2022

This pull request introduces 2 alerts when merging 20dc383 into bc557dd - view on LGTM.com

new alerts:

  • 2 for Unused import

Pending refining + better unit conversions
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 18, 2022

This pull request introduces 2 alerts when merging 9b04d30 into bc557dd - view on LGTM.com

new alerts:

  • 2 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 21, 2022

This pull request introduces 2 alerts when merging 54bbd9e into bc557dd - view on LGTM.com

new alerts:

  • 2 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 7, 2022

This pull request introduces 2 alerts when merging 61849cc into 6bcdfcc - view on LGTM.com

new alerts:

  • 2 for Unused import

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link
Contributor

@chrisjonesBSU chrisjonesBSU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented a couple questions, and some things to watch out for. I haven't tried using this yet, I'll be sure to do that ASAP.

gmso/external/convert_hoomd.py Outdated Show resolved Hide resolved
gmso/external/convert_hoomd.py Outdated Show resolved Hide resolved
gmso/external/convert_hoomd.py Outdated Show resolved Hide resolved
gmso/external/convert_hoomd.py Outdated Show resolved Hide resolved
return angle_forces


def _parse_dihedral_forces(top, potential_types, potential_refs, base_units):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this funciton, do we need to make sure any values for angles are in the correct units, similar to what you're doing in the _parse_angles function?

@daico007 daico007 marked this pull request as ready for review January 26, 2023 22:08
@chrisjonesBSU
Copy link
Contributor

chrisjonesBSU commented Jan 31, 2023

I tested this out in a notebook. It seems to be working well for methane. See my comments above about making sure we use radians.

I also tried it out with ethane so we have dihedrals, and the performance suffers quite a bit.

import mbuild as mb
import gmso
import forcefield_utilities as ffutils
from gmso.external.convert_hoomd import to_hoomd_forcefield 
from gmso.external.convert_mbuild import from_mbuild
from gmso.parameterization import apply

mol = mb.load("CC", smiles=True)
box = mb.packing.fill_box(compound=mol, n_compounds=20, box=[2,2,2])
oplsaa = ffutils.FoyerFFs().load('oplsaa').to_gmso_ff()
gmso_box = from_mbuild(box)
apply(top=gmso_box, forcefields=oplsaa, identify_connections=True)

forcefield = to_hoomd_forcefield(top=gmso_box)

When I use 40 methane molecules the to_hoomd_forcefield call takes about 200ms to run

When I use 20 ethane molecules the to_hoomd_forcefield call takes takes over 4 minutes to run

The slow down must be somewhere in parsing dihedrals, since methane doesn't contain any.

@daico007
Copy link
Member Author

Good catch! I also just realized the performance issue last night but haven't pinpoint where the bottleneck is. From your comment, I will start with the dihedral type parsing.

@chrisjonesBSU
Copy link
Contributor

I think we should implement auto_scale in the same way it's done in create_hoomd_forcefield in mBuild. Has there been any discussion about this that I missed, or is this something that is potentially handled elsewhere in GMSO?

@chrisjonesBSU
Copy link
Contributor

I think we should implement auto_scale in the same way it's done in create_hoomd_forcefield in mBuild. Has there been any discussion about this that I missed, or is this something that is potentially handled elsewhere in GMSO?

Also, I can plan on contributing this part of the PR if it is something we want in to_hoomd_forcefield

@daico007
Copy link
Member Author

@chrisjonesBSU please do! I don't think we make any decision about the autoscale stuff and I just forgot about that altogether

Copy link
Contributor

@chrisjonesBSU chrisjonesBSU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Auto scale seems to be working as expected. What do you think about returning the base_units dict along with the forcefield? This is basically what we do in mbuild's create_hoomd_forcefield.

Also, looks like there are a few commented out lines from when you were troubleshooting things.

I think this is ready after we decide whether or not to include base_units in the return statement.

gmso/external/convert_hoomd.py Outdated Show resolved Hide resolved
gmso/external/convert_hoomd.py Show resolved Hide resolved
gmso/external/convert_hoomd.py Outdated Show resolved Hide resolved
gmso/external/convert_hoomd.py Outdated Show resolved Hide resolved
gmso/external/convert_hoomd.py Outdated Show resolved Hide resolved
Copy link
Contributor

@chrisjonesBSU chrisjonesBSU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One note to fix the import, I'll push this change real quick, just wanted to document it.

gmso/external/convert_hoomd.py Outdated Show resolved Hide resolved
Make r_cut a requried arguement for to_hoomd_forcefield.
Temporarily disable lrc_cache since it's could behave in a weird manners.
Fix related issues with the inputs of check_compatibility
gmso/external/convert_hoomd.py Dismissed Show dismissed Hide dismissed
@@ -4,6 +4,7 @@
import os
import sys
import textwrap
from tempfile import TemporaryFile

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'TemporaryFile' is not used.
except ImportError:
has_gsd = False

try:
import hoomd

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'hoomd' is not used.
Comment on lines +23 to +27
from gmso.utils.sorting import (
natural_sort,
sort_connection_members,
sort_member_types,
)

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'natural_sort' is not used.
@@ -1,5 +1,8 @@
"""Determine if the parametrized gmso.topology can be written to an engine."""
from functools import lru_cache

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'lru_cache' is not used.
from unyt.array import allclose_units

from gmso.core.views import PotentialFilters
from gmso.exceptions import GMSOError, NotYetImplementedWarning

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'GMSOError' is not used.
Copy link
Contributor

@chrisjonesBSU chrisjonesBSU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! It seems to be working great. Thanks for working on this @daico007.

@daico007
Copy link
Member Author

Cool, thank you for reviewing @chrisjonesBSU! I will let this sit a bit in case anyone want to drop a comment/review. I will merge this PR early next week.

oplsaa = ffutils.FoyerFFs().load("oplsaa").to_gmso_ff()
top = apply(top, oplsaa, remove_untyped=True)

gmso_snapshot, snapshot_base_units = to_hoomd_snapshot(

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable gmso_snapshot is not used.
oplsaa = ffutils.FoyerFFs().load("oplsaa").to_gmso_ff()
top = apply(top, oplsaa, remove_untyped=True)

gmso_snapshot, snapshot_base_units = to_hoomd_snapshot(

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable snapshot_base_units is not used.
gmso_snapshot, snapshot_base_units = to_hoomd_snapshot(
top, base_units=base_units
)
gmso_forces, forces_base_units = to_hoomd_forcefield(

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable gmso_forces is not used.
gmso_snapshot, snapshot_base_units = to_hoomd_snapshot(
top, base_units=base_units
)
gmso_forces, forces_base_units = to_hoomd_forcefield(

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable forces_base_units is not used.
@daico007 daico007 merged commit 35aa77f into mosdef-hub:main Apr 24, 2023
8 of 10 checks passed
@daico007 daico007 mentioned this pull request May 2, 2023
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

2 participants