Define Pydantic schema on Quantity - #178
Conversation
a819b49 to
d09b154
Compare
Fixes Add Pydantic depedency to Python project metadata Update lockfile
a6b977c to
3b36116
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #178 +/- ##
==========================================
+ Coverage 89.83% 91.37% +1.53%
==========================================
Files 6 6
Lines 187 232 +45
==========================================
+ Hits 168 212 +44
- Misses 19 20 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
…efine-pydantic-schema
j-wags
left a comment
j-wags
left a comment
There was a problem hiding this comment.
I spoke with the OpenFE leads and concluded that we should move forward with this approach. Please let me know once this is ready for review!
for more information, see https://pre-commit.ci
…efine-pydantic-schema
j-wags
left a comment
j-wags
left a comment
There was a problem hiding this comment.
Thanks @mattwthompson - This looks good to me. A few comments but none blocking. Thanks for reviewing this with me live today.
- (not blocking) If the previous version supported list of lists of uneven length (eg
[[1,2],[3]]) it would be good to either ensure that works after these changes, or test that defining/serializing/deserializing such a value raises an error.
| # TODO: I think this is necessary for handling unit-wrapped arrays, but it is | ||
| # not so performant. Scalar quantities can be directly serialized to much | ||
| # shorter strings |
There was a problem hiding this comment.
(not blocking) If performance is a concern, maybe add a test defining/serializing/deserializing a big array quantity and test performance with a timeout (even if the current performance is acceptable, this could prevent other changes in the future from exacerbating performance issues)
There was a problem hiding this comment.
Valid concern but deferring this until it's diagnosed as an issue
| # Quantity constructor, but "{"magnitude": 0.9, "units": "nanometer"}" cannot | ||
| # as it needs to be unwrapped. A better solution would require a better way | ||
| # of serializing unit-wrapped arrays to JSON | ||
| if "{" in v: |
There was a problem hiding this comment.
(not blocking) 👍 I was certain I was about to see an eval statement after this, nice work not needing that.
|
|
||
| @requires_package("openmm") | ||
| def _to_openmm(self) -> "openmm.unit.Quantity": | ||
| def _to_openmm(self) -> openmm.unit.Quantity: |
There was a problem hiding this comment.
(not blocking) You'd asked me during synchronous review to flag this.
There was a problem hiding this comment.
I was concerned this would break things if OpenMM isn't installed, but that's not the case because I turned on deferred annotations (from __future__ import annotations)
[openff-units] pixi run -e py312pint24 python define-pydantic-schema ✱
Python 3.12.13 | packaged by conda-forge | (main, Mar 5 2026, 17:06:14) [Clang 19.1.7 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import openmm
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'openmm'
>>> from openff.units import *
>>> Quantity("1 picosecond")
<Quantity(1, 'picosecond')>
3 participants
Description
This is a possible solution for #177
and openforcefield/openff-interchange#1346 and arguably a better design from the ground-up than what we had in
openff-modelsTodos
strQuantitylistor similar iterabledictnumpy.ndarrayStatus