-
Notifications
You must be signed in to change notification settings - Fork 262
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
Rename hydra heads #903
Rename hydra heads #903
Conversation
This is WIP, I have only renamed the EqV2 heads, but please go ahead and have a look and provide comments/suggestions and I can rename/edit all other heads |
@deprecated( | ||
"equiformer_v2_energy_head (EquiformerV2EnergyHead) class is deprecated in favor of equiformerV2_scalar_head (EqV2ScalarHead)" | ||
) | ||
@registry.register_model("equiformer_v2_energy_head") |
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.
should we just add @registry.register_model("equiformer_v2_energy_head")
on top of EqV2ScalarHead so that we dont need this extra class?
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 left the additional deprecated classes for configs that specified the module path (ie model: fairchem.core.models.equiformer_v2.equiformer_v2.EquiformerV2EnergyHead
).
If we dont mind just breaking backwards compatibility, I'd say then remove the deprecated classes here, and the name from the registry as well
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.
oh ic, ok let's leave but lets remember to remove it in a future cleanup task then
The current code has specific names for energy and forces heads along with the keys in the prediction results.
This makes it a bit convoluted to use existing heads for another valid property. For example using the
EquiformerV2EnergyHead
to predict another scalar property. As it is now, we can achieve this by miss-labeling outputs and dataset targets as "energy".This PR renames prediction heads more broadly (ie Energy -> Scalar, Forces -> Vector, etc). It also adds an output_name attribute to heads so that the prediction dictionary can be set to return arbitrary property names