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

Improve documentation of from_openmm and related features #1008

Merged
merged 5 commits into from
Jul 17, 2024

Conversation

mattwthompson
Copy link
Member

Description

In part resolves #1004 #1005 #1007

@mattwthompson mattwthompson changed the base branch from main to develop July 12, 2024 20:48
@openforcefield openforcefield deleted a comment from review-notebook-app bot Jul 12, 2024
Copy link

codecov bot commented Jul 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.28%. Comparing base (6a1fb9e) to head (a3d93b4).
Report is 14 commits behind head on develop.

Additional details and impacted files

@mattwthompson mattwthompson changed the title Doc from openmm Improve documentation of from_openmm and related features Jul 15, 2024
@mattwthompson
Copy link
Member Author

@mattwthompson mattwthompson marked this pull request as ready for review July 16, 2024 19:55
@mattwthompson
Copy link
Member Author

Could I get a quick once-over (aka find my typos, even though I edited it once myself)?

Copy link
Collaborator

@Yoshanuikabundi Yoshanuikabundi left a comment

Choose a reason for hiding this comment

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

Two typos, and then a bunch of optional rephrasings to reduce parentheticals and keep a single logical flow. Looks good, this is very useful info! I've also suggested using a Sphinx cross-reference to link to the appropriate page, and I think the note might need to be moved to different functions?

docs/using/edges.md Outdated Show resolved Hide resolved

### Modified masses are ignored

The OpenFF Toolkit does not support isotopes or modifiying masses from the values defined in the periodic table. In the `Topology` and `Molecule` classes, particles masses are defined only by their atomic number. When topologies are read from OpenMM, the particle mass is ignored and the atomic number of the element is read and used to define the atomic properties.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternate:

Suggested change
The OpenFF Toolkit does not support isotopes or modifiying masses from the values defined in the periodic table. In the `Topology` and `Molecule` classes, particles masses are defined only by their atomic number. When topologies are read from OpenMM, the particle mass is ignored and the atomic number of the element is read and used to define the atomic properties.
The OpenFF Toolkit does not support isotopes or modifying masses from the values defined in the periodic table. In the `Topology` and `Molecule` classes, particles masses are defined only by their atomic number. When topologies are read from OpenMM, the particle mass is ignored and the atomic number of the element is read and used to define the atomic properties.

docs/using/edges.md Outdated Show resolved Hide resolved
docs/using/edges.md Outdated Show resolved Hide resolved
docs/using/edges.md Outdated Show resolved Hide resolved
docs/using/edges.md Outdated Show resolved Hide resolved
openff/interchange/components/interchange.py Outdated Show resolved Hide resolved
Co-authored-by: Josh A. Mitchell <[email protected]>
@mattwthompson
Copy link
Member Author

Closes #1006

@mattwthompson mattwthompson merged commit ad8ddae into develop Jul 17, 2024
21 checks passed
@mattwthompson
Copy link
Member Author

Note this is going into the 0.4 line, so these changes won't be reflected in 0.3 releases

@mattwthompson mattwthompson deleted the doc-from-openmm branch July 17, 2024 14:00
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.

from_openmm doesn't take System masses
2 participants