-
Notifications
You must be signed in to change notification settings - Fork 7
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.
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
Adapt models to handle generic targets #386
Conversation
…zation problems
…ients of `energy`
Co-authored-by: Philip Loche <[email protected]>
e158719
to
0c207fc
Compare
daa06e0
to
f598aab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are only tests for saopbpnn where the functionality works, not for catching the errors in the other models, maybe add tests
if not ( | ||
target.is_scalar | ||
and target.quantity == "energy" | ||
and "atom" not in target.layout.block(0).samples.names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here a stupid question becouse i do not know the code. when atom
is in target.layout.block(0).samples.names
,
target.quantity
can still be energy
? if this is the case i can see a case where the error becomes:
"GAP only supports total-energy-like outputs, but a energy was provided"
which do not make much sense.
If this is the case split this error message for the case "atom" not in target.layout.block(0).samples.names
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right. I will split the error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently I see this:
raise ValueError(
"PET only supports total-energy-like outputs, "
f"but a {target.quantity} was provided"
)
should we instead propagate the request for generic targets to PET right away, given that PET does support 1) multidimensional targets, 2) targets per atom and per structure, and 3) both sum and mean aggregations?
@spozdn The goal for this PR is to make sure that each type of target is handled by each architecture. We allowed spherical vector targets for SOAP-BPNN, but this is just to test the correctness of the whole infrastructure. Also GAP and the alchemical model could in principle fit a larger range of targets than those allowed here, as can PET, but that would be for a different PR. When it comes to PET, we're having a lot of integration issues even with the basic energy targets, so I wouldn't add more burden by adding more options until the current integration issues are solved and people want to use the extra features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for me it is fine, for the discussion with PET I leave to you and sergey
Thanks! Regarding the extra features for PET, I will make sure to open an issue. Waiting for #399 to be merged to avoid adding more problems to that PR |
This PR defines the capabilities of each one of the current model in the documentation and performs the corresponding checks in the code. Part of #364.
Contributor (creator of pull-request) checklist
📚 Documentation preview 📚: https://metatrain--386.org.readthedocs.build/en/386/