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

Rank2 tensor head #792

Merged
merged 13 commits into from
Aug 17, 2024
Merged

Rank2 tensor head #792

merged 13 commits into from
Aug 17, 2024

Conversation

lbluque
Copy link
Collaborator

@lbluque lbluque commented Aug 2, 2024

This PR adds a rank 2 tensor head for equiformerV2.

It is based on the implementations in the stress_trainer branch:
https://github.com/FAIR-Chem/fairchem/blob/stress_trainer/ocpmodels/models/utils/outer_block.py

TODO

  • Add unit tests

@lbluque lbluque requested review from misko and mshuaibii August 2, 2024 19:57
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 56.25000% with 49 lines in your changes missing coverage. Please review.

Files Patch % Lines
...ore/models/equiformer_v2/prediction_heads/rank2.py 55.45% 49 Missing ⚠️
Files Coverage Δ
...airchem/core/models/equiformer_v2/equiformer_v2.py 88.47% <100.00%> (ø)
...ore/models/equiformer_v2/prediction_heads/rank2.py 55.45% <55.45%> (ø)

... and 1 file with indirect coverage changes

misko
misko previously approved these changes Aug 7, 2024
Copy link
Collaborator

@misko misko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! No stress, LGTM! 🚀

@misko misko added enhancement New feature or request minor Minor version release labels Aug 16, 2024
@misko misko requested review from rayg1234, wood-b and misko August 16, 2024 20:42
misko
misko previously approved these changes Aug 16, 2024
@misko
Copy link
Collaborator

misko commented Aug 16, 2024

I ran comparisons between the original rank2 stress tensor code that @lbluque has and compared it to the Hydra equivalent model.
https://fairwandb.org/fairchem/mptrj/reports/Stress-head-MPTraj-original-vs-Hydra-version--Vmlldzo0NDQ4NQ

The results initially were different, however we traced the issue down to a difference in using amp in the head. This PR moves all heads to by default be used without AMP.

@@ -189,6 +189,10 @@ def no_weight_decay(self) -> list:


class HeadInterface(metaclass=ABCMeta):
@property
def use_amp(self):
return False
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the default be to not use it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When i talked to @wood-b he mentioned that we should have it off by default, it shouldnt change much memory usage and from our experiments it only helps with performance metrics

@misko misko self-requested a review August 16, 2024 22:19
@misko misko enabled auto-merge August 17, 2024 00:09
@misko misko added this pull request to the merge queue Aug 17, 2024
Merged via the queue into main with commit e380c66 Aug 17, 2024
9 checks passed
@misko misko deleted the rank2-head branch August 17, 2024 02:00
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.

3 participants