deprecate(ModelOutput.quantity): mark quantity field as deprecated by HaoZeke · Pull Request #199 · metatensor/metatomic

HaoZeke · 2026-04-03T09:28:27Z

The quantity field is no longer required for unit conversion since the unit parser determines dimensions from the expression itself.

Changes:

Add deprecated documentation to C++ header
Add TORCH_WARN in C++ setter when quantity is non-empty
Add DeprecationWarning in Python when quantity validation runs
Update documentation with deprecation notice
Add deprecation entries to CHANGELOG

Contributor (creator of pull-request) checklist

Tests updated (for new features and bugfixes)?
Documentation updated (for new features)?
Issue referenced (for PRs that solve an issue)?

Reviewer checklist

CHANGELOG updated with public API or any other important changes?

Luthaf · 2026-04-07T08:02:56Z

    yield
    captured = capfd.readouterr()
    # the code should not print anything to stdout or stderr
+    # except for expected deprecation warnings


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

I really prefer this, it's much cleaner.

The point of checking warnings explicitly is similar to checking for exceptions. IMO it is not enough to check that "an exception has been thrown at some point", we want to make sure that the exception/warning we expect is produced exactly by the one line of code we expect to throw/emit it

Luthaf · 2026-04-07T08:04:39Z

    yield
    captured = capfd.readouterr()
    # the code should not print anything to stdout or stderr
+    # except for expected deprecation warnings


Same as above

Luthaf · 2026-04-07T08:05:21Z

    yield
    captured = capfd.readouterr()
    # the code should not print anything to stdout or stderr
+    # except for expected deprecation warnings


Same as above

Luthaf · 2026-04-07T13:52:08Z

+    # consume the once-per-process quantity deprecation warning from C++
+    captured = capfd.readouterr()
+    if captured.err:
+        assert "ModelOutput.quantity is deprecated" in captured.err


We can change the code in the test LJ to remove the need to catch this warning

metatensor/lj-test#17

Could you do it over there, then we merge and use the new version here directly, instead of needing to update the code twice? Since we always pin lj-test with a specific commit, there should be no downstream issues doing this.

…-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`.

…nd migrate call sites Pol's fca4e05 kept the existing `TORCH_WARN_DEPRECATION` in `set_per_atom` / `get_per_atom` intentionally: the new `sample_kind` API is preferred and `per_atom` is the deprecated shim. This commit mirrors the conftest/filterwarnings pattern the morfixes branch uses for `ModelOutput.quantity`, applied only to `per_atom` (no overlap with #199 -- this PR does not depend on morfixes). Tests / packaging - `tests/conftest.py` in all three subpackages: regex-strip the C++ `TORCH_WARN_DEPRECATION` line for `per_atom` from stderr in the autouse `fail_test_with_output` fixture. The regex tolerates 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` in all three subpackages: ignore the matching Python `DeprecationWarning` so tests exercising the field directly do not blow up under `-W error`. - `tests/outputs.py`: drop the two `assert message in captured.err` sections from `test_sample_kind`. On Torch 2.3 the C++ warning lands on stderr directly and the conftest regex scrubs it; on Torch 2.11 `PyErr_WarnEx` + the filterwarnings ignore leaves stderr empty. Asserting a specific stderr shape mid-test was torch-version-brittle. The `pytest.warns(match=...)` guards remain, so the shim is still verified. Drop the now-unused `capfd` parameter. Internal call site migration - `metatomic_torch/metatomic/torch/heat_flux.py`: the four `ModelOutput(..., per_atom=...)` call sites for masses/velocities/ energies/heat_flux output all use `sample_kind="atom"/"system"` now, so the wrapper does not emit warnings from its own construction path. - `metatomic_torch/metatomic/torch/model.py`: the dedup check in `_get_requested_inputs` compares `sample_kind` instead of reading the deprecated `per_atom` property. - `metatomic_torch/tests/heat_flux.py`, `metatomic_ase/tests/heat_flux.py`, `metatomic_ase/tests/calculator.py::test_inputs_different_units`: migrate all `ModelOutput(..., per_atom=...)` to `sample_kind`.

My earlier switch to `HaoZeke/lj-test@f56f6b4` broke every test that computes an energy via the reference LJ model: the fork drops `quantity="energy"` from every `ModelOutput(...)` (forward-looking for the morfixes #199 `quantity` deprecation), which causes `AtomisticModel.forward()` to hit the early `continue` in if declared.quantity == "" or requested.quantity == "": continue and skip the `unit_conversion_factor(declared.unit, requested.unit)` step that the engine needs when asking for energy in `"ev"` while the model declares `"eV"`. Downstream ASE tests see ~100x-wrong energies. `metatensor/lj-test@eea52c6` is the existing upstream sample_kind branch commit: it already migrates every `ModelOutput(per_atom=...)` to `ModelOutput(sample_kind=...)` and every `outputs[...].per_atom` read to `outputs[...].sample_kind == "atom"` so the runtime deprecation path is not hit, but it keeps `quantity=...` intact so unit conversion still runs. That is the right pin for this PR's scope (per_atom only, no quantity overlap). Both `install_lj_tests` and the inline `pip install` in the `torch-tests` env are bumped to `eea52c6`. Bump again once that branch lands on lj-test main and gets a stable SHA.

HaoZeke force-pushed the morfixes branch 3 times, most recently from 7b73a2a to 409b32c Compare April 5, 2026 00:50

HaoZeke requested a review from Luthaf April 5, 2026 01:06

Luthaf reviewed Apr 7, 2026

View reviewed changes

HaoZeke force-pushed the morfixes branch from 37c3728 to 116aa3e Compare April 7, 2026 15:05

HaoZeke force-pushed the morfixes branch from 5d36f30 to 6e73fcb Compare April 15, 2026 21:58

HaoZeke mentioned this pull request Apr 16, 2026

chore: suppress more warnings metatensor/lj-test#17

Open

HaoZeke requested a review from Luthaf April 16, 2026 12:06

HaoZeke mentioned this pull request Apr 17, 2026

Added sample_kind to ModelOutput, without breaking changes #168

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deprecate(ModelOutput.quantity): mark quantity field as deprecated#199

deprecate(ModelOutput.quantity): mark quantity field as deprecated#199
HaoZeke wants to merge 1 commit intometatensor:mainfrom
HaoZeke:morfixes

HaoZeke commented Apr 3, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Luthaf Apr 7, 2026

Uh oh!

HaoZeke Apr 17, 2026

Uh oh!

Luthaf Apr 17, 2026

Uh oh!

Uh oh!

Luthaf Apr 7, 2026

Uh oh!

Luthaf Apr 7, 2026

Uh oh!

Luthaf Apr 7, 2026

Uh oh!

HaoZeke Apr 16, 2026

Uh oh!

Luthaf Apr 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

HaoZeke commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contributor (creator of pull-request) checklist

Reviewer checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

HaoZeke commented Apr 3, 2026 •

edited

Loading