Skip to content

Fix the default velocity unit of ase #152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Luthaf merged 2 commits into from
Feb 5, 2026
Merged

Fix the default velocity unit of ase #152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
472c56b
Fix the default velocity unit of `ase`
GardevoirX Feb 4, 2026
de11e07
Use `ase.units` in unit conversion tests
GardevoirX Feb 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions metatomic-torch/src/model.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1141,6 +1141,7 @@ static std::map<std::string, Quantity> KNOWN_QUANTITIES = {
{"A/fs", 1e1},
{"m/s", 1e6},
{"nm/ps", 1e3},
{"(eV/u)^(1/2)", 101.80506},
Copy link
Member

@HaoZeke HaoZeke Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno if this is the right place but why not

(eV/u) **0.5

Do we recognize that too? I can't remember

Copy link
Contributor Author

@GardevoirX GardevoirX Feb 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need a page to introduce the units that we support? Like exposing things here to users
https://github.com/metatensor/metatomic/blob/main/metatomic-torch%2Fsrc%2Fmodel.cpp#L1078-1154

Copy link
Member

@Luthaf Luthaf Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these should be documented in https://docs.metatensor.org/metatomic/latest/torch/reference/misc.html#known-quantities-and-units for now.

(eV/u) **0.5

No, we don't recognise that for now. I don't think we ever should, although we could introduce (eV/u)^0.5 at some point. For now I think this is fine

Copy link
Contributor Author

@GardevoirX GardevoirX Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these should be documented in https://docs.metatensor.org/metatomic/latest/torch/reference/misc.html#known-quantities-and-units for now.

(eV/u) **0.5

No, we don't recognise that for now. I don't think we ever should, although we could introduce (eV/u)^0.5 at some point. For now I think this is fine

Can we link this page to every page of the quantities? I did remember that we have such a table but failed to find it out... BTW we haven't update charge into this table yet

Copy link
Member

@HaoZeke HaoZeke Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean... we could just do an expression parser or something, there's gotta be something out there already, with sympy and even openmm taking expression strings.

The point is rather than explicitly enumerating it'd be easier to just define the operators and be done.

Copy link
Contributor Author

@GardevoirX GardevoirX Feb 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I also thought about this a little bit, if this is possible, we can utilize the unit stored in the info of the TensorMap and do unit calculations when functions in metatensor.operations are called, which makes our tensor more tensor-like. but maybe it's just nobody want to spend time on this

Maybe opening an issue on it is the best action at this moment

Copy link
Member

@Luthaf Luthaf Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean... we could just do an expression parser or something, there's gotta be something out there already, with sympy and even openmm taking expression strings.

Yes, this is the plan. I'll open an issue

Copy link
Member

@Luthaf Luthaf Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#154

}, {
// alternative names
}}},
Expand Down
2 changes: 1 addition & 1 deletion python/metatomic_torch/metatomic/torch/ase_calculator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ def _get_charges(atoms: ase.Atoms) -> np.ndarray:
"velocities": {
"quantity": "velocity",
"getter": ase.Atoms.get_velocities,
"unit": "nm/fs",
"unit": "(eV/u)^(1/2)",
},
"charges": {
"quantity": "charge",
Expand Down
4 changes: 3 additions & 1 deletion python/metatomic_torch/tests/ase_calculator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,6 +866,8 @@ def test_additional_input(atoms):

expected = ARRAY_QUANTITIES[name]["getter"](atoms).reshape(values.shape)
if name == "velocities":
expected *= 10.0 # ase velocity is in nm/fs
expected /= (
ase.units.Angstrom / ase.units.fs
) # ase velocity is in (eV/u)^(1/2) and we want A/fs

assert np.allclose(values, expected)
Loading