Implement new standard outputs for atomistic models #654
Conversation
frostedoyster
force-pushed
the
new-properties
branch
3 times, most recently
from
1fae531
to
ea5b5ef
Compare
frostedoyster
force-pushed
the
new-properties
branch
from
ea5b5ef
to
7803363
Compare
There was a problem hiding this comment.
This is changing way too many things at the same time. Could you have one PR for energy-ensemble, one PR for dipole and one PR changing the ASE calculator?
The new outputs need code to check that the data matches the specification in https://github.com/lab-cosmo/metatensor/blob/master/python/metatensor-torch/metatensor/torch/atomistic/outputs.py
| @@ -1032,6 +1034,29 @@ static std::unordered_map<std::string, Quantity> KNOWN_QUANTITIES = { | |||
| {"J", "Joule"}, | |||
| {"Ry", "Rydberg"}, | |||
| }}}, | |||
| {"energy_ensemble", Quantity{/* name */ "energy", /* baseline */ "eV", { | |||
There was a problem hiding this comment.
This should not be needed, energy_ensemble should be able to re-use the energy quantity and associated units
| {"dipole", Quantity{/* name */ "dipole", /* baseline */ "D", { | ||
| {"Debye", 1.0}, | ||
| {"Coulomb-meter", 1000.0}, | ||
| {"atomic units", 0.03674932247495664}, |
There was a problem hiding this comment.
I'm not sure I love having a space inside the unit =/
| :param properties_to_store: list of model outputs to store as results of the ASE | ||
| calculator at every step. This is useful when you want to store properties | ||
| that are not used in the propagation of the dynamics and/or are not standard | ||
| ASE properties ('energy', 'forces', 'stress', 'stresses', 'dipole', | ||
| 'charges', 'magmom', 'magmoms', 'free_energy', 'energies'). These properties | ||
| will be available as ``atoms.calc.results['<stored_property>']``. |
There was a problem hiding this comment.
Is this typical in ASE? The idea for properties not part of the standard was to use the run_model function.
In general, this does not seems to be required for dipole, since ASE already supports them as property.
| }, { | ||
| // alternative names | ||
| {"D", "Debye"}, | ||
| {"C-m", "Coulomb-meter"}, |
There was a problem hiding this comment.
Should this be C.m? That's what https://en.wikipedia.org/wiki/Electric_dipole_moment uses
| {"J", "Joule"}, | ||
| {"Ry", "Rydberg"}, | ||
| }}}, | ||
| {"dipole", Quantity{/* name */ "dipole", /* baseline */ "D", { |
There was a problem hiding this comment.
do we want to call this electric_dipole?
|
Ok, I will open different PRs |
|
Thx Guillaume & Filippo. Feel free to re-assign me for ASE changes or ensemble stuff. 馃憤 |
This PR implements ensembles of energies as a new standard output for atomistic models (#650). It also makes the necessary changes to the ASE calculator to make this property (which is not standard in ASE) available to the user. In addition, the dipole is also registered as a new property and added to the ASE calculator
TODO: actual conversion factors for dipoles
Contributor (creator of pull-request) checklist
Reviewer checklist
馃摎 Download documentation preview for this pull-request