Skip to content
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

Merged
merged 7 commits into from
Nov 8, 2024
Merged

Rename hydra heads #903

merged 7 commits into from
Nov 8, 2024

Conversation

lbluque
Copy link
Collaborator

@lbluque lbluque commented Nov 4, 2024

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

@lbluque
Copy link
Collaborator Author

lbluque commented Nov 4, 2024

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

@lbluque lbluque added enhancement New feature or request minor Minor version release labels Nov 4, 2024
Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 79.14110% with 68 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/fairchem/core/_cli_hydra.py 75.00% 16 Missing ⚠️
src/fairchem/core/common/utils.py 69.04% 13 Missing ⚠️
src/fairchem/core/common/distutils.py 53.84% 12 Missing ⚠️
.../fairchem/core/scripts/convert_hydra_to_release.py 71.79% 11 Missing ⚠️
...fairchem/core/models/equiformer_v2/heads/scalar.py 84.84% 5 Missing ⚠️
src/fairchem/core/components/runner.py 84.00% 4 Missing ⚠️
...em/core/models/equiformer_v2/eqv2_to_eqv2_hydra.py 75.00% 3 Missing ⚠️
...fairchem/core/models/equiformer_v2/heads/vector.py 90.00% 3 Missing ⚠️
...m/core/common/relaxation/optimizers/lbfgs_torch.py 0.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/fairchem/core/_cli.py 68.11% <100.00%> (ø)
src/fairchem/core/common/flags.py 100.00% <100.00%> (ø)
...airchem/core/models/equiformer_v2/equiformer_v2.py 79.51% <100.00%> (-1.06%) ⬇️
...irchem/core/models/equiformer_v2/heads/__init__.py 100.00% <100.00%> (ø)
.../fairchem/core/models/equiformer_v2/heads/rank2.py 100.00% <100.00%> (ø)
...core/models/equiformer_v2/weight_initialization.py 100.00% <100.00%> (ø)
...m/core/common/relaxation/optimizers/lbfgs_torch.py 15.75% <0.00%> (ø)
...em/core/models/equiformer_v2/eqv2_to_eqv2_hydra.py 92.72% <75.00%> (-5.24%) ⬇️
...fairchem/core/models/equiformer_v2/heads/vector.py 90.00% <90.00%> (ø)
src/fairchem/core/components/runner.py 84.00% <84.00%> (ø)
... and 5 more

... and 6 files with indirect coverage changes

@deprecated(
"equiformer_v2_energy_head (EquiformerV2EnergyHead) class is deprecated in favor of equiformerV2_scalar_head (EqV2ScalarHead)"
)
@registry.register_model("equiformer_v2_energy_head")
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

Copy link
Collaborator

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

@lbluque lbluque added this pull request to the merge queue Nov 8, 2024
Merged via the queue into main with commit 834fbd6 Nov 8, 2024
12 of 13 checks passed
@lbluque lbluque deleted the rename-heads branch November 8, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor Minor version release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants