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

Replace unyt.amu with Unit("amu") to perserve mass values from mBuild #759

Merged
merged 4 commits into from
Sep 4, 2023

Conversation

chrisjonesBSU
Copy link
Contributor

@chrisjonesBSU chrisjonesBSU commented Aug 28, 2023

This is a quick PR that changes how we use amu units from Unyt. There is some inconsistent behavior with how Unyt handles amu specifically compared to other units. See the issue here

In from_mbuild I believe the goal is to copy over the particle mass as is, but to add on amu units since mBuild uses amu for mass. But, when we do this using u.amu we end up converting the mass to kg. This happens because u.amu is already a unyt_quantity that is in kg. In the issue, there is a suggestion to use Unit('amu') instead.

Example: You can see u.amu does not have the same behavior as u.g or u.g/u.mol, but Unit("amu") does.

>>> import unyt as u
>>> from unyt import Unit
>>> u.amu.units
kg
>>> u.amu
unyt_quantity(1.66053892e-27, 'kg')
>>> Unit("amu")
amu
>>> u.g
g
>>> u.g/u.mol
g/mol

Here is what the change would look like in gmso. IMO, keeping the mass values the same as they are in mBuild, and actually using units of amu instead of kg is preferable.

Code snippet:

>>> import gmso
>>> import mbuild as mb
>>> from gmso.external import from_mbuild
>>> methane = mb.load("C", smiles=True)
>>> top = from_mbuild(methane)

Using unyt.amu (current behavior):

>>> for p in methane:
...     print(p.mass) 
12.011
1.008
1.008
1.008
1.008

>>> for site in top.sites:
...     print(site.mass)
... 
1.9944732980130996e-26 kg
1.673823232368e-27 kg
1.673823232368e-27 kg
1.673823232368e-27 kg
1.673823232368e-27 kg

Using Unit("amu"):

>>> for p in methane:
...     print(p.mass) 
12.011
1.008
1.008
1.008
1.008

>>> for site in top.sites:
...     print(site.mass)
... 
12.011 amu
1.008 amu
1.008 amu
1.008 amu
1.008 amu
>>> 

@chrisjonesBSU chrisjonesBSU changed the title Unyt amu Replace unyt.amu with Unit("amu") to perserve mass values from mBuild Aug 28, 2023
@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (5f98454) 92.07% compared to head (3ffbfcb) 92.07%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #759   +/-   ##
=======================================
  Coverage   92.07%   92.07%           
=======================================
  Files          67       67           
  Lines        6892     6893    +1     
=======================================
+ Hits         6346     6347    +1     
  Misses        546      546           
Files Changed Coverage Δ
gmso/external/convert_mbuild.py 93.75% <100.00%> (+0.03%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@daico007 daico007 left a comment

Choose a reason for hiding this comment

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

LGTM!

@daico007 daico007 merged commit d584ffa into mosdef-hub:main Sep 4, 2023
24 checks passed
@chrisjonesBSU chrisjonesBSU deleted the unyt_amu branch September 5, 2023 03:44
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