Added sample_kind to ModelOutput, without breaking changes#168
Added sample_kind to ModelOutput, without breaking changes#168
sample_kind to ModelOutput, without breaking changes#168Conversation
|
This is ready for a final look. I checked that one can take the conda |
| ModelOutputHolder( | ||
| std::string quantity, | ||
| std::string unit, | ||
| bool per_atom_, | ||
| torch::optional<bool> per_atom_, | ||
| std::vector<std::string> explicit_gradients_, | ||
| std::string description_ | ||
| std::string description_, | ||
| torch::optional<std::string> sample_kind = torch::nullopt |
There was a problem hiding this comment.
So we should keep the previous constructor as-is for compatibility, and add another one that takes only sample_kind.
This one is useful for the Python API bindings, but I would do something like
ModelOutputHolder(string quantity, string unit, torch::IValue _3, string[] explicit_gradients, string description, string? sample_kind, bool? per_atom)
And then allow to pass sample_kind as a positional argument in the same place as the previous per_atom, while still supporting keyword arguments, and disallowing setting both as the same time
There was a problem hiding this comment.
I don't understand what is the use of torch::IValue _3, I have done the changes without it and it seems fine
There was a problem hiding this comment.
Basically allowing someone to do ModelOutput("quantity", "unit", true) as well as ModelOutput("quantity", "unit", "atom_pair"), while also allowing for ModelOutput("quantity", "unit", per_atom=true) and ModelOutput("quantity", "unit", sample_kind="atom")
There was a problem hiding this comment.
But for this to work I imagine I have to process the _3 argument no? Or this is magic? If you have something specific in mind please go ahead with the changes
| "Sample_kind '", sample_kind, "' is not officially supported. ", | ||
| "This means that metatomic doesn't natively understand how to deal ", | ||
| "with such outputs. If this is a mistake, pass one of the supported ", |
There was a problem hiding this comment.
Hmm, not sure if a warning is the solution here =/ I'd rather make this an error.
There was a problem hiding this comment.
The error will be raised when validating data. My idea is that this will help people play with new sample kinds before they are officially supported, which is nice
| } | ||
| } else { | ||
| /// We don't validate values for other cases for now | ||
| return; |
There was a problem hiding this comment.
I think we should do some validation here =)
There was a problem hiding this comment.
How can we know the expected samples? This depends on the system and the neighbor list options 😅
There was a problem hiding this comment.
I imagine there is some way, do you want to do it? :) This feels daunting to me since it is my first C++ PR haha
There was a problem hiding this comment.
This depends on the system and the neighbor list options 😅
Hmm, true =/ I guess we can only validate that the names are corrects and the indices in-bounds. I can do this if you want!
There was a problem hiding this comment.
Yes go ahead 😅 The system that you receive for validation doesn't contain the neighbor list?
Co-authored-by: Guillaume Fraux <guillaume.fraux@epfl.ch>
2 participants
This PR substitutes #165.
It also implements
sample_kind, with the short-term goal of enabling per-pair targets, but in this case in a way that doesn't break backward compatibility (new code using oldModelOutputmight fail, but old code using the newModelOutputwon't). This is achieved by keeping the possibility to passper_atomas an argument, and setting/getting it as a property, even though from now on the only thingModelOutputstores issample_kind.Thanks to @Luthaf for letting me know that one can set optional arguments in torch exportable classes :)
If you agree with merging this one I will finalize it with some more tests ✌️
📚 Download documentation for this pull-request
⚙️ Download Python wheels for this pull-request (you can install these with pip)
📚 Download documentation for this pull-request
⚙️ Download Python wheels for this pull-request (you can install these with pip)
📚 Download documentation for this pull-request
⚙️ Download Python wheels for this pull-request (you can install these with pip)
📚 Download documentation for this pull-request
⚙️ Download Python wheels for this pull-request (you can install these with pip)
📚 Download documentation for this pull-request