-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add HOOMD Supports #696
Conversation
carried over code from gsd writer, instead of saving the gsd file, just return the snapshot object
This pull request introduces 3 alerts when merging 49f62ff into 691371c - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging 1486fd0 into bc557dd - view on LGTM.com new alerts:
|
Codecov ReportPatch coverage:
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
... 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. |
This pull request introduces 3 alerts when merging b682255 into bc557dd - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 20dc383 into bc557dd - view on LGTM.com new alerts:
|
Pending refining + better unit conversions
This pull request introduces 2 alerts when merging 9b04d30 into bc557dd - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 54bbd9e into bc557dd - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 61849cc into 6bcdfcc - view on LGTM.com new alerts:
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. |
There was a problem hiding this 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.
There was a problem hiding this 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
return angle_forces | ||
|
||
|
||
def _parse_dihedral_forces(top, potential_types, potential_refs, base_units): |
There was a problem hiding this comment.
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?
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.
When I use 40 methane molecules the When I use 20 ethane molecules the The slow down must be somewhere in parsing dihedrals, since methane doesn't contain any. |
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. |
I think we should implement |
Also, I can plan on contributing this part of the PR if it is something we want in |
@chrisjonesBSU please do! I don't think we make any decision about the |
…into support_hoomd_sim
There was a problem hiding this 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.
There was a problem hiding this 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.
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
from gmso.utils.conversions import ( | ||
convert_opls_to_ryckaert, | ||
convert_ryckaert_to_opls, | ||
) |
Check notice
Code scanning / CodeQL
Unused import
@@ -4,6 +4,7 @@ | |||
import os | |||
import sys | |||
import textwrap | |||
from tempfile import TemporaryFile |
Check notice
Code scanning / CodeQL
Unused import
except ImportError: | ||
has_gsd = False | ||
|
||
try: | ||
import hoomd |
Check notice
Code scanning / CodeQL
Unused import
from gmso.utils.sorting import ( | ||
natural_sort, | ||
sort_connection_members, | ||
sort_member_types, | ||
) |
Check notice
Code scanning / CodeQL
Unused import
@@ -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
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
There was a problem hiding this 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.
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
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
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
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
[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.