Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update unit checks in Topology.to_file #1207

Merged
merged 1 commit into from
Mar 11, 2022

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Mar 4, 2022

Today I needed to write a PDB file from a Topology and I found that the docstring, behavior, and tests don't really line up. This is probably my fault, being the last person to touch this code.

>>> from openff.toolkit import Topology
>>> print(Topology.to_file.__doc__)

        Save coordinates and topology to a PDB file.

        Reference: https://github.com/openforcefield/openff-toolkit/issues/502

        Notes:

        1. This doesn't handle virtual sites (they're ignored)
        2. Atom numbering may not remain same, for example if the atoms
           in water are numbered as 1001, 1002, 1003, they would change
           to 1, 2, 3. This doesn't affect the topology or coordinates or
           atom-ordering in any way.
        3. Same issue with the amino acid names in the pdb file, they are
           not returned.

        Parameters
        ----------
        filename : str
            name of the pdb file to write to
        positions : n_atoms x 3 numpy array or openmm.unit.Quantity-wrapped n_atoms x 3 iterable
            Can be an openmm 'quantity' object which has atomic positions as a list of Vec3s along with associated units, otherwise a 3D array of UNITLESS numbers are considered as "Angstroms" by default
        file_format : str
            Output file format. Case insensitive. Currently only supported value is "pdb".

The docstring doesn't indicate that openff.units.unit.Quantity is accepted, and the logic of how it's handled is a little dangerous (again from yours truly). In particular, there is no logic that handles positions tagged with our Pint-style units, so it's automatically stripped of its units when it gets to PDBFile.writeFile. This silently works if they're in Angstroms but will silently break if they're in nanometers.

This PR updates the behavior to explicitly and more safely check the type of the positions input and tests some of the allowed use cases.

@codecov
Copy link

codecov bot commented Mar 4, 2022

Codecov Report

❗ No coverage uploaded for pull request base (topology-biopolymer-refactor@0b0999e). Click here to learn what that means.
The diff coverage is n/a.

@mattwthompson mattwthompson marked this pull request as ready for review March 8, 2022 15:04
Copy link
Member

@j-wags j-wags left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks, @mattwthompson!

@mattwthompson mattwthompson merged commit 59f35b8 into topology-biopolymer-refactor Mar 11, 2022
@mattwthompson mattwthompson deleted the to-file-units branch March 11, 2022 23:45
mattwthompson added a commit that referenced this pull request Mar 24, 2022
commit d34fe857e7cf28c312141bca58ff3b81173eecb0
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 13:27:43 2022 -0500

    Fix bad merge

commit 7d309f2
Merge: 9273dbb b373e40
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 11:42:15 2022 -0500

    Merge remote-tracking branch 'upstream/add-topology' into add-topology

commit 9273dbb
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 10:44:12 2022 -0500

    Fix mypy

commit 4ec3ea7
Merge: b8a5793 45fb642
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 10:41:13 2022 -0500

    Merge remote-tracking branch 'upstream/topology-biopolymer-refactor' into add-topology

commit 45fb642
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Wed Mar 23 21:34:20 2022 -0500

    Raise descriptive error when OpenEye stereochemistry assumptions fail (#1218)

commit fc3d637
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Wed Mar 23 21:33:52 2022 -0500

    Performance improvements in from_openeye (#1217)

    OpenMM quantities are generally as quick and lightweight as
    corresponding NumPy arrays, but Pint units tend to be slower. Most of
    these changes simply defer unit tagging until a final step, carrying
    values with implicit units in some internal functions where the units
    are known and can be expected not to vary.

commit b373e40
Merge: b8a5793 f297b64
Author: Jeff Wagner <jwagnerjpl@gmail.com>
Date:   Wed Mar 23 16:04:17 2022 -0700

    Merge branch 'topology-biopolymer-refactor' into add-topology

commit f297b64
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Wed Mar 23 17:21:16 2022 -0500

    Fix incorrect assignment of TIP3P parameters in Sage (#1200) (#1220)

    * Add test for TIP3P charges with Sage

    * Do not uniquify SMARTS in LibraryChargeHandler

    * Also test standalone TIP3P file

    * Consolidate tip3p charge tests

    * black

    * Update release history

    Co-authored-by: j-wags <jwagnerjpl@gmail.com>

    Co-authored-by: j-wags <jwagnerjpl@gmail.com>

commit a8d8b09
Author: Jeff Wagner <jwagnerjpl@gmail.com>
Date:   Mon Mar 21 14:44:21 2022 -0700

    Resolve conflicts (#1224)

commit 8d4a0a8
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 18:27:50 2022 -0600

    Remove un-used Topology attributes (#1213)

    * Remove un-used Topology attributes

    * Lint

    * Update release history

commit ba879b2
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 18:26:50 2022 -0600

    Remove some internal calls to deprecated API points (#1204)

    * Remove some internal calls to deprecated API points

    * Fixes

    * Add test that deprecated Topology methods still exist

    * Fix broken list population

commit 59f35b8
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 17:45:05 2022 -0600

    Avoid silent/implicit units checks in Topology.to_file, update docstring (#1207)

commit 46893b0
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 16:15:22 2022 -0600

    Add Electrostatics tag to `test_forcefields/tip3p.offxml` (#1188)

    * Add Electrostatics section to data/test_forcefields/tip3p.offxml

    * Update release history

commit 0c42148
Author: Jeff Wagner <jwagnerjpl@gmail.com>
Date:   Fri Mar 11 13:43:59 2022 -0800

    fix #837 (#1214)

commit 0cb3a72
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 10 14:34:37 2022 -0600

    Fix formatting in a `.assign_partial_charges` docstring (#1212)

commit b8a5793
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Feb 9 11:58:02 2022 -0600

    Remove old mendeleev import

commit 373262a
Merge: 31c7755 0beef75
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Feb 9 11:38:27 2022 -0600

    Merge remote-tracking branch 'upstream/topology-biopolymer-refactor' into add-topology

commit 31c7755
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Feb 9 11:30:23 2022 -0600

    Update Topology.__add__/__iadd__, add tests, fix annotations

commit fc12de1
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Tue Feb 8 15:33:41 2022 -0600

    Add first implementation of Topoogy.__add__

commit 0321d99
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 12:30:12 2022 -0600

    Remove Atom.element

commit a2e7bf9
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 12:25:03 2022 -0600

    Fix virtual site creation

commit a383a0a
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 11:26:19 2022 -0600

    Fix setting masses while adding OpenMM particles

commit 37e86db
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 10:20:30 2022 -0600

    Fix more typos

commit 6555475
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 09:33:13 2022 -0600

    Fix typos, tests

commit c529ee0
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Jan 26 18:49:04 2022 -0600

    Draft a refactor to openff.units.elements
mattwthompson added a commit that referenced this pull request May 11, 2022
* Squashed commit of the following:

commit d34fe857e7cf28c312141bca58ff3b81173eecb0
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 13:27:43 2022 -0500

    Fix bad merge

commit 7d309f2
Merge: 9273dbb b373e40
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 11:42:15 2022 -0500

    Merge remote-tracking branch 'upstream/add-topology' into add-topology

commit 9273dbb
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 10:44:12 2022 -0500

    Fix mypy

commit 4ec3ea7
Merge: b8a5793 45fb642
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 24 10:41:13 2022 -0500

    Merge remote-tracking branch 'upstream/topology-biopolymer-refactor' into add-topology

commit 45fb642
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Wed Mar 23 21:34:20 2022 -0500

    Raise descriptive error when OpenEye stereochemistry assumptions fail (#1218)

commit fc3d637
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Wed Mar 23 21:33:52 2022 -0500

    Performance improvements in from_openeye (#1217)

    OpenMM quantities are generally as quick and lightweight as
    corresponding NumPy arrays, but Pint units tend to be slower. Most of
    these changes simply defer unit tagging until a final step, carrying
    values with implicit units in some internal functions where the units
    are known and can be expected not to vary.

commit b373e40
Merge: b8a5793 f297b64
Author: Jeff Wagner <jwagnerjpl@gmail.com>
Date:   Wed Mar 23 16:04:17 2022 -0700

    Merge branch 'topology-biopolymer-refactor' into add-topology

commit f297b64
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Wed Mar 23 17:21:16 2022 -0500

    Fix incorrect assignment of TIP3P parameters in Sage (#1200) (#1220)

    * Add test for TIP3P charges with Sage

    * Do not uniquify SMARTS in LibraryChargeHandler

    * Also test standalone TIP3P file

    * Consolidate tip3p charge tests

    * black

    * Update release history

    Co-authored-by: j-wags <jwagnerjpl@gmail.com>

    Co-authored-by: j-wags <jwagnerjpl@gmail.com>

commit a8d8b09
Author: Jeff Wagner <jwagnerjpl@gmail.com>
Date:   Mon Mar 21 14:44:21 2022 -0700

    Resolve conflicts (#1224)

commit 8d4a0a8
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 18:27:50 2022 -0600

    Remove un-used Topology attributes (#1213)

    * Remove un-used Topology attributes

    * Lint

    * Update release history

commit ba879b2
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 18:26:50 2022 -0600

    Remove some internal calls to deprecated API points (#1204)

    * Remove some internal calls to deprecated API points

    * Fixes

    * Add test that deprecated Topology methods still exist

    * Fix broken list population

commit 59f35b8
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 17:45:05 2022 -0600

    Avoid silent/implicit units checks in Topology.to_file, update docstring (#1207)

commit 46893b0
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Fri Mar 11 16:15:22 2022 -0600

    Add Electrostatics tag to `test_forcefields/tip3p.offxml` (#1188)

    * Add Electrostatics section to data/test_forcefields/tip3p.offxml

    * Update release history

commit 0c42148
Author: Jeff Wagner <jwagnerjpl@gmail.com>
Date:   Fri Mar 11 13:43:59 2022 -0800

    fix #837 (#1214)

commit 0cb3a72
Author: Matt Thompson <mattwthompson@protonmail.com>
Date:   Thu Mar 10 14:34:37 2022 -0600

    Fix formatting in a `.assign_partial_charges` docstring (#1212)

commit b8a5793
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Feb 9 11:58:02 2022 -0600

    Remove old mendeleev import

commit 373262a
Merge: 31c7755 0beef75
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Feb 9 11:38:27 2022 -0600

    Merge remote-tracking branch 'upstream/topology-biopolymer-refactor' into add-topology

commit 31c7755
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Feb 9 11:30:23 2022 -0600

    Update Topology.__add__/__iadd__, add tests, fix annotations

commit fc12de1
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Tue Feb 8 15:33:41 2022 -0600

    Add first implementation of Topoogy.__add__

commit 0321d99
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 12:30:12 2022 -0600

    Remove Atom.element

commit a2e7bf9
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 12:25:03 2022 -0600

    Fix virtual site creation

commit a383a0a
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 11:26:19 2022 -0600

    Fix setting masses while adding OpenMM particles

commit 37e86db
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 10:20:30 2022 -0600

    Fix more typos

commit 6555475
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Thu Jan 27 09:33:13 2022 -0600

    Fix typos, tests

commit c529ee0
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Jan 26 18:49:04 2022 -0600

    Draft a refactor to openff.units.elements

* Preserve constraints in `Topology.__add__`

* Update release history
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants