compatibility follow up with metatensor 0.9.0rc6 + metatomic 0.1.12rc2 + featomic 0.6.6#1128
compatibility follow up with metatensor 0.9.0rc6 + metatomic 0.1.12rc2 + featomic 0.6.6#1128sofiia-chorna wants to merge 7 commits into
Conversation
| for target_name in targets.keys(): | ||
| # Check if the target is an energy: | ||
| if model_outputs[target_name].quantity == "energy": | ||
| if targets[target_name].quantity == "energy": |
There was a problem hiding this comment.
Is this intended to match on energy, energy_ensemble, and energy_uncertaintyoutputs, or onlyenergy`?
in the second case, this could be done better using
base_target_name = target_name.split("/")[0]
if base_target_name == "energy"
i.e. checking the output name, handling both energy and energy/pbe0. This way we could also remove the quantity field from metatrain
There was a problem hiding this comment.
yes but i am wondering that the custom energy names will be missed in this case, like mtt::U0
There was a problem hiding this comment.
There really should not be a point to use something like this nowadays =) This kind of naming was used before we got variants on the models outputs.
There was a problem hiding this comment.
Hmmmmm if we remove quantity, old options.yaml will sometimes not work as Sofiia says for example when you pass a target called mtt::U0 (which is what the tutorials do).
Although I agree that the default behavior is a bit strange, everything by default is quantity: "energy" (even spherical targets), it is set here:
metatrain/src/metatrain/utils/omegaconf.py
Lines 118 to 130 in cf1b015
which makes
quantity: "energy" by itself quite useless, and the code needs to perform further checks to know that it is dealing with an energy, like here:metatrain/src/metatrain/utils/data/dataset.py
Lines 793 to 798 in cf1b015
So yeah I agree this should go away but I'm a bit scared of touching it tbh
|
We also need to remove |
|
Not in this PR, but when updating metatomic yes! You'll get a proper deprecation warning here. |
|
so, in this PR, we just keep a |
f807e95 to
102610b
Compare
|
ok, locally i have those errors with the latest release candidates: tox -e tests tox -e gap-tests tox -e pet-tests tox -e soap-bpnn-tests tox -e phace-tests tox -e llpr-tests tox -e flashmd-tests tox -e flashmd-symplectic-tests tox -e mace-tests tox -e dpa3-tests |
|
@Luthaf @pfebrer
=> actual error: from: metatrain/src/metatrain/experimental/dpa3/model.py Lines 393 to 397 in 08ec341 to fix it, we should probably sort beforehand with |
|
This one is a bug, fix incoming as soon as tests pass on my machine! |
|
tested with |
a) i am wondering it is not backward compatible? so we should update it together with dependencies bump PR? b) this one is strange, not sure exactly what changes cause this: in can investigate deeper of course or hand it over to LLPR people 😁 |
|
Both LLPR errors are strange. I don't see how any of the changes in metatensor/metatomic could cause these =/ |
|
ok the first issue is related to the check: metatrain/src/metatrain/utils/data/target_info.py Lines 760 to 762 in 08ec341 where there is only i'd just add |
418e692 to
afcb313
Compare
afcb313 to
ecb59c2
Compare
|
With the new release candidate, it will be possible to replace all TensorBlock recreations with This can't be done before updating the metatensor dependency, but keep it in mind to later replace as many of these "recreations" as possible, to make the code a bit nicer :) |
|
Let's update this PR to use all the new releases instead of release candidates, and we use it as a PR to upgrade the dependencies of metatrain 👍 |
|
featomic and featomic-torch are still release candidates i suppose |
|
sorry i will reopen it with the new branch, wanted to simply rename it to correspond to the deps bump 😄 |
Will be released as soon as CI is happy on metatensor/featomic#435 |
tested versions:
some fixes:
2da11ce - use
targets[name].quantityas deprecated on ModelOutput903841e -
is_auxiliary_outputalso matches singular"feature"(renamed in metatomic)f0d92ea a66b3f5 and afcb313- recreate or copy tensorblocks
f2d96df -
unit="unitless"=>""not fixed here:
in llpr:
compute_cholesky_decompositionerror, see #1128 (comment)📚 Documentation preview 📚: https://metatrain--1128.org.readthedocs.build/en/1128/