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 more notes about unit conversions to the hoomd functions #1062

Merged
merged 14 commits into from
Jan 5, 2023

Conversation

chrisjonesBSU
Copy link
Contributor

PR Summary:

This PR addresses #1057. It provides further explanation about the behind-the-scenes conversion of units, and an example of how to use the reference value parameters to convert to another unit system (mosdef units in this case).

Let me know if anything looks incorrect, or isn't clear.

PR Checklist


  • Includes appropriate unit test(s)
  • Appropriate docstring(s) are added/updated
  • Code is (approximately) PEP8 compliant
  • Issue(s) raised/addressed?

Copy link
Contributor

@CalCraven CalCraven left a comment

Choose a reason for hiding this comment

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

Few formatting changes! Otherwise, this documentation is sorely needed.

To convert the distance units from Angstrom to nm:
use ref_distance = 10 (angstroms/nm)

You can also use the auto_scale parameter to convert distance, energy
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to give an example of this as well. I'd move line 98 up above the Examples section, then make a full new:

Examples
-----------

Section with all of these individual examples. See something like this as a format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I consolidated the examples section here. I see what you're saying about the examples in that other file. Do you think I should write out a full example from imports to creating an mb.Compound, etc.. or just leave these examples specific to picking a value for the reference parameters.

mbuild/formats/hoomd_forcefield.py Outdated Show resolved Hide resolved
mbuild/formats/hoomd_snapshot.py Outdated Show resolved Hide resolved
mbuild/formats/hoomd_snapshot.py Outdated Show resolved Hide resolved
mbuild/formats/hoomd_snapshot.py Outdated Show resolved Hide resolved
mbuild/formats/hoomd_snapshot.py Outdated Show resolved Hide resolved
mbuild/formats/hoomd_snapshot.py Outdated Show resolved Hide resolved
mbuild/formats/hoomd_snapshot.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 15, 2022

Codecov Report

Base: 90.49% // Head: 78.21% // Decreases project coverage by -12.28% ⚠️

Coverage data is based on head (c12c9f2) compared to base (4df37e7).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1062       +/-   ##
===========================================
- Coverage   90.49%   78.21%   -12.29%     
===========================================
  Files          61       57        -4     
  Lines        6157     5513      -644     
===========================================
- Hits         5572     4312     -1260     
- Misses        585     1201      +616     
Impacted Files Coverage Δ
mbuild/formats/hoomd_forcefield.py 0.00% <ø> (-78.46%) ⬇️
mbuild/formats/hoomd_snapshot.py 0.00% <ø> (-79.02%) ⬇️
mbuild/formats/hoomd_simulation.py 0.00% <0.00%> (-78.72%) ⬇️
mbuild/compound.py 83.00% <0.00%> (-13.86%) ⬇️
mbuild/utils/decorators.py 88.88% <0.00%> (-11.12%) ⬇️
mbuild/utils/io.py 80.17% <0.00%> (-3.45%) ⬇️
mbuild/conversion.py
mbuild/__init__.py
... and 2 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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 c752cd1 into mosdef-hub:main Jan 5, 2023
@chrisjonesBSU chrisjonesBSU deleted the fix/hoomd_docs branch January 6, 2023 00:56
thangckt added a commit to thangckt/mbuild that referenced this pull request Jan 9, 2023
Add more notes about unit conversions to the hoomd functions (mosdef-hub#1062)
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.

3 participants