deprecate(ModelOutput.quantity): mark quantity field as deprecated#199
Open
HaoZeke wants to merge 1 commit intometatensor:mainfrom
Open
deprecate(ModelOutput.quantity): mark quantity field as deprecated#199HaoZeke wants to merge 1 commit intometatensor:mainfrom
HaoZeke wants to merge 1 commit intometatensor:mainfrom
Conversation
HaoZeke
force-pushed
the
morfixes
branch
3 times, most recently
from
7b73a2a to
409b32c
Compare
Luthaf
reviewed
Apr 7, 2026
| /// quantity of the output (e.g. energy, dipole, …). If this is an empty | ||
| /// string, no unit conversion will be performed. | ||
| /// @deprecated This field is no longer required for unit conversion. | ||
| /// The unit parser determines dimensions from the expression itself. |
Member
There was a problem hiding this comment.
Can you do a [[deprecated]] atribute instead, both here and on the setter?
|
|
||
| void ModelOutputHolder::set_quantity(std::string quantity) { | ||
| if (!quantity.empty()) { | ||
| TORCH_WARN( |
Member
There was a problem hiding this comment.
Should be a torch deprecation warning, there is a special macro for it
| yield | ||
| captured = capfd.readouterr() | ||
| # the code should not print anything to stdout or stderr | ||
| # except for expected deprecation warnings |
Member
There was a problem hiding this comment.
These should be explicitly caught at the place where they are emitted in the test, by using the capfd fixture in the tests. I'm doing this in a couple of place already
Comment on lines
+513
to
+517
| # Note: Deprecation warning for quantity is emitted from C++ when | ||
| # ModelOutput is constructed with non-empty quantity. | ||
| # We don't warn here because TorchScript can't handle | ||
| # DeprecationWarning. | ||
|
|
Member
There was a problem hiding this comment.
we should remove the code here that uses quantity
| yield | ||
| captured = capfd.readouterr() | ||
| # the code should not print anything to stdout or stderr | ||
| # except for expected deprecation warnings |
| yield | ||
| captured = capfd.readouterr() | ||
| # the code should not print anything to stdout or stderr | ||
| # except for expected deprecation warnings |
Luthaf
reviewed
Apr 7, 2026
Comment on lines
+50
to
+53
| # consume the once-per-process quantity deprecation warning from C++ | ||
| captured = capfd.readouterr() | ||
| if captured.err: | ||
| assert "ModelOutput.quantity is deprecated" in captured.err |
Member
There was a problem hiding this comment.
We can change the code in the test LJ to remove the need to catch this warning
…-based unit conversion The quantity field on ModelOutput is no longer required: the unit expression parser determines dimensions directly, so unit conversion no longer needs the quantity hint. C++ (metatomic-torch) - Mark `quantity()` getter and `set_quantity()` setter with `[[deprecated]]` so callers get a compile-time warning. - In `set_quantity`, emit `TORCH_WARN_DEPRECATION` when a non-empty quantity is provided so JSON-deserialized models with `quantity` set surface the deprecation at runtime. Python (metatomic-torch) - Drop the quantity-vs-quantity gating and mismatch check in `model.py`; gate unit conversion on `unit` being non-empty instead. The unit parser already validates dimensional consistency. - Add a `.. deprecated::` block to the `quantity` property docstring. Tests / packaging - conftest.py for all three subpackages: regex-strip the C++ `TORCH_WARN_DEPRECATION` line from stderr in the autouse fail-on-output fixture, tolerating both Torch 2.3 (`[W model.cpp:N]`) and Torch 2.11+ (`[W<MMDD> HH:MM:SS.fractZ model.cpp:N]`) prefixes. Any other unexpected stderr still fails the test. - `pyproject.toml` filterwarnings: ignore the matching Python `DeprecationWarning` so tests that exercise the field directly do not blow up under `-W error`. - Update tests/examples that constructed `ModelOutput(quantity=...)` to drop the parameter where the test is not specifically about quantity validation. - Update C++ models.cpp test to allow the deprecation warning. Changelog: add a Deprecated entry for `ModelOutput.quantity`.
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
2 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.
The quantity field is no longer required for unit conversion since the unit parser determines dimensions from the expression itself.
Changes:
Contributor (creator of pull-request) checklist
Reviewer checklist