Fix the default velocity unit of ase
#152
Conversation
Luthaf
left a comment
Luthaf
left a comment
There was a problem hiding this comment.
could you also check the unit for momenta?
yeah this one is correct, bascially |
| {"A/fs", 1e1}, | ||
| {"m/s", 1e6}, | ||
| {"nm/ps", 1e3}, | ||
| {"(eV/u)^(1/2)", 101.80506}, |
There was a problem hiding this comment.
Dunno if this is the right place but why not
(eV/u) **0.5
Do we recognize that too? I can't remember
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.5at 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
3 participants
The default unit is actually
(eV/u)^(1/2)but I wrongly set it asnm/fs.For reference, see https://ase-lib.org/ase/units.html, and also their implementation of
_maxwellboltzmanndistributionhttps://gitlab.jsc.fz-juelich.de/kesselheim1/ase/-/blob/e240cec52abeb150b1b5cefa69888b7b60489878/ase/md/velocitydistribution.py#L14-21Contributor (creator of pull-request) checklist
Reviewer checklist