search

choderalab / modelforge

Infrastructure to implement and train NNPs
https://modelforge.readthedocs.io/en/latest/
MIT License
9 stars 4 forks source link

Adding unit checks to the public API #56

Closed wiederm closed 5 months ago

wiederm commented 5 months ago

Description

Currently, we are not enforcing units at several public interfaces. This PR aims to fix this and enforces the internal use of the openmm unit system. This PR will close issues #54 and #32. The forward call of the models will check for units attached to positions and convert them to nanometers if possible. If no units are attached, it will assume that the units are in nanometers and print a warning to stderr.

Todos

Notable points that this PR has either accomplished or will accomplish.

Status

codecov-commenter commented 5 months ago

Codecov Report

Merging #56 (8ef70ef) into main (ed63a24) will decrease coverage by 2.03%. The diff coverage is 90.00%.

Additional details and impacted files
chrisiacovella commented 5 months ago

We had been using openff-units already in the package. I don't think we should switch to openmm-units for the public facing API. Openff-units should be more flexible (as we can easily convert to/from openmm units if we need) and has a convenient function unit.parse_expression, to make it easily to reassign units from the HDF5 files (which are encoded as strings).

I think we should actually probably switch to openff-units in Chiron, just doing conversions from the openmm test systems (but that is a conversation for a different day in a different repo).

chrisiacovella commented 5 months ago

After quick discussion, we are going to use openff-units. I'll be grabbing a copy of the branch and making these changes.

wiederm commented 5 months ago

Thanks @chrisiacovella for these changes!

Sorry for introducing yet another unit package! Thanks for catching that in time and making the necessary modifications!