-
Couldn't load subscription status.
- Fork 4
Update variants #107
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
base: main
Are you sure you want to change the base?
Update variants #107
Conversation
e4de588 to
d9d5c42
Compare
7ca4e3d to
b52ed40
Compare
| if uncertainty_threshold is not None: | ||
| self._energy_uq_key = pick_output( | ||
| "energy_uncertainty", outputs, resolved_variants["energy_uncertainty"] |
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.
The pick_output will raise an error here if the uncertainty_threshold is a number (which is the default) but the model does not have an energy_uncertainty in its outputs. I think this is what one actually wants. If a user desires an uncertainty warning but the model can't do it - we should error.
But this change kinda leads to that one now have to set the uncertainty_threshold to None for all models without an uncertainty output. Before we silently ignored this. We can also change the default of uncertainty_threshold from 0.1 to None.
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.
So I think the behavior should be
- by default, if the model has an
energy_uncertaintyoutput, we use it with a threshold of 0.1 eV/atom without the user having to do anything. - if the users specifies a variant for
energy_uncertaintybut does not touch the threshold, we use the same 0.1 eV/atom threshold - the user can explicitly opt-out of the warning by setting the threshold to None.
In the code, we could try to check if there is an energy_uncertainty output before calling pick_output to resolve it.
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.
Okay we can do a check if energy_uncertainty or a variant is in any of the output keys.
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.
I would only check if one variant is present, the one that would be used by pick_outout.
b52ed40 to
9ae6826
Compare
| if uncertainty_threshold is not None: | ||
| self._energy_uq_key = pick_output( | ||
| "energy_uncertainty", outputs, resolved_variants["energy_uncertainty"] |
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.
So I think the behavior should be
- by default, if the model has an
energy_uncertaintyoutput, we use it with a threshold of 0.1 eV/atom without the user having to do anything. - if the users specifies a variant for
energy_uncertaintybut does not touch the threshold, we use the same 0.1 eV/atom threshold - the user can explicitly opt-out of the warning by setting the threshold to None.
In the code, we could try to check if there is an energy_uncertainty output before calling pick_output to resolve it.
9ae6826 to
238ce63
Compare
| void ModelCapabilitiesHolder::set_outputs(torch::Dict<std::string, ModelOutput> outputs) { | ||
| std::unordered_map<std::string, std::unordered_set<std::string>> variants; | ||
|
|
||
| std::unordered_map<std::string, std::vector<std::string>> units; |
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.
I would not call this units, variants was fine though
| ); | ||
|
|
||
| // check for variant description warning | ||
| #if TORCH_VERSION_MAJOR >= 2 && TORCH_VERSION_MINOR >= 0 |
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.
we require torch >=2.1, so this check is not needed
This PR adds a couple of updates to the variants:
"energy/A","energy/B"without"energy"being present.descriptionattribute toModelOutputs.pick_variantfunction that will pick a variant based on aname(like"energy"), aModelCapabilitiesobject and an optionaldesired_variant. The function will try to pick the user choice and if not given either pick the non-variant or error giving the provided variances. I am not happy with the parameter naming and order. We can discuss itTODO
pick_variantin ase calculatorReviewer checklist
📚 Download documentation for this pull-request