feat(units): add an expression parser#173
Open
HaoZeke wants to merge 7 commits intometatensor:mainfrom
Open
Conversation
HaoZeke
force-pushed
the
feat/unit-expression-parser
branch
3 times, most recently
from
92bf24c to
0fe0ef7
Compare
Contributor
|
Thanks a lot! I think it would be better if we can use the new functionality to check if the |
GardevoirX
reviewed
Mar 4, 2026
HaoZeke
added a commit
to HaoZeke/metatomic
that referenced
this pull request
Mar 4, 2026
Address PR metatensor#173 review feedback from GardevoirX: - Add s, second, ms, us, ns, ps with full-word aliases to time tokens - Add tests verifying ModelOutput rejects mismatched quantity/unit dims - Add tests for standalone micro sign (U+00B5) -> Dalton resolution - Update docs token table and doxygen with new time unit coverage - Fix stray dash in RST list-table Dimensionless row
Luthaf
reviewed
Mar 9, 2026
docs/src/torch/reference/misc.rst
Outdated
Comment on lines
+23
to
+24
| Dimensional compatibility is verified automatically; no ``quantity`` parameter | ||
| is needed. Token lookup is case-insensitive, and whitespace is ignored. |
Member
There was a problem hiding this comment.
Should this be documented as part of unit_conversion_factor directly?
docs/src/torch/reference/misc.rst
Outdated
| - Tokens | ||
| - Notes | ||
| * - **Length** | ||
| - ``angstrom`` (``a``), ``bohr``, ``nm`` (``nanometer``), ``meter`` (``m``), ``cm`` (``centimeter``), ``mm`` (``millimeter``), ``um`` (``micrometer``) |
Member
There was a problem hiding this comment.
Suggested change
| - ``angstrom`` (``a``), ``bohr``, ``nm`` (``nanometer``), ``meter`` (``m``), ``cm`` (``centimeter``), ``mm`` (``millimeter``), ``um`` (``micrometer``) | |
| - ``angstrom`` (``A``), ``bohr``, ``nm`` (``nanometer``), ``meter`` (``m``), ``cm`` (``centimeter``), ``mm`` (``millimeter``), ``um`` (``micrometer``) |
This should explicitly list µm as well. I'm also not sure why the main unit is sometimes meter sometimes cm. I would use the full name every time, and put the abbreviation in parenthesis.
docs/src/torch/reference/misc.rst
Outdated
| - ``angstrom`` (``a``), ``bohr``, ``nm`` (``nanometer``), ``meter`` (``m``), ``cm`` (``centimeter``), ``mm`` (``millimeter``), ``um`` (``micrometer``) | ||
| - | ||
| * - **Energy** | ||
| - ``ev``, ``mev``, ``hartree``, ``ry`` (``rydberg``), ``joule`` (``j``), ``kcal``, ``kj`` |
Member
There was a problem hiding this comment.
Suggested change
| - ``ev``, ``mev``, ``hartree``, ``ry`` (``rydberg``), ``joule`` (``j``), ``kcal``, ``kj`` | |
| - ``eV``, ``meV``, ``Hartree``, ``Ry`` (``rydberg``), ``Joule`` (``J``), ``kcal``, ``kJ`` |
let's use the usual upper case version of the units in the docs
docs/src/torch/reference/misc.rst
Outdated
| - ``ev``, ``mev``, ``hartree``, ``ry`` (``rydberg``), ``joule`` (``j``), ``kcal``, ``kj`` | ||
| - ``kcal`` and ``kj`` are bare (not per-mol); write ``kcal/mol`` for the per-mole unit | ||
| * - **Time** | ||
| - ``s`` (``second``), ``ms`` (``millisecond``), ``us`` (``microsecond``), ``ns`` (``nanosecond``), ``ps`` (``picosecond``), ``fs`` (``femtosecond``) |
docs/src/torch/reference/misc.rst
Outdated
Comment on lines
+57
to
+58
| Known quantities | ||
| ^^^^^^^^^^^^^^^^ |
Member
There was a problem hiding this comment.
Long term, I think we'll want to remove the user-provided quantity, and just check that an "energy" output has an unit with the dimension of an energy.
metatomic-torch/tests/models.cpp
Outdated
|
|
||
| CHECK_THROWS_WITH(options->set_length_unit("unknown"), | ||
| StartsWith("unknown unit 'unknown' for length") | ||
| StartsWith("unknown unit token 'unknown'") |
Member
There was a problem hiding this comment.
I don't think user care about tokens, so why mention it in the error message?
metatomic-torch/CHANGELOG.md
Outdated
|
|
||
| ### Changed | ||
|
|
||
| - `validate_unit` now accepts arbitrary parseable expressions and validates |
Member
There was a problem hiding this comment.
validate_unit is not public and should not be mentioned here =)
| _unit_conversion_factor_v2 = torch.ops.metatomic.unit_conversion_factor_v2 | ||
| _unit_conversion_factor_v1 = torch.ops.metatomic.unit_conversion_factor | ||
|
|
||
| def unit_conversion_factor(*args, **kwargs): |
Member
There was a problem hiding this comment.
does this work with TorchScript? I thought *args, **kwargs is not supported
Replace the old string-matching unit conversion with a Shunting-Yard expression parser that supports *, /, ^, and parentheses. Unit code extracted to units.hpp/cpp per review feedback. Micro sign handling uses explicit base_units entries instead of global normalization.
Remove standalone micro sign test (no longer normalizes globally),
add micro sign microsecond tests instead. Update error message
assertions ("unknown unit" not "unknown unit token"). Add comment
explaining why models.cpp test uses valid quantity/unit strings.
Move full unit expression documentation to the Doxygen comment on unit_conversion_factor in units.hpp (renders via autofunction). Remove redundant standalone sections from misc.rst, keeping only the known-quantities table and deprecation note.
HaoZeke
force-pushed
the
feat/unit-expression-parser
branch
from
01198d7 to
eb8e28a
Compare
Remove validate_unit from CHANGELOG (not public API). Move unit_conversion_factor docstring from documentation.py to the Python function in __init__.py (documentation.py is only for C++ ops).
to_lower() now skips non-ASCII bytes, preventing macOS locale from mangling UTF-8 micro sign (0xB5 -> 'u'). Restore unit_conversion_factor in documentation.py since __init__.py imports it for Sphinx builds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #154.
Replaced the per-quantity lookup tables with a Shunting-Yard expression parser
that works on arbitrary compound unit strings in the spirit of lumol.
Each token resolves to an SI conversion factor and a 5-element dimension vector
[L, T, M, Q, Theta]. The parser composes these through multiplication, division,
and exponentiation. Conversion factor between two expressions = ratio of their
SI factors after verifying dimension equality.
API changes
Expression syntax
Operators:
*(multiply),/(divide),^(power),()(grouping).Whitespace ignored. Case-insensitive. Numeric literals allowed in exponents.
Fractional exponents via parenthesized division:
^(1/2).Token table
Single flat
unordered_mapwith 30+ entries covering length (angstrom, bohr, nm,m, cm, mm, um), energy (eV, meV, hartree, ry, joule, kcal, kJ), time (fs, ps),
mass (u, kg, g, electronmass), charge (e, coulomb), dimensionless (mol), and
derived (hbar).
Notes
kelvinis NOT in the token table because temperature conversions betweenoffset-based scales (Celsius, Fahrenheit) are non-multiplicative.
DIM_TEMPERATUREexists as dimension [0,0,0,0,1] for potential future use butno tokens currently carry it. (maybe once we do an API break, can revisit during
mini-metatomic)Contributor (creator of pull-request) checklist
Reviewer checklist