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

Remove detaches to enable gradient-based force EquiformerV2 models. #779

Closed
wants to merge 4 commits into from

Conversation

kyonofx
Copy link
Collaborator

@kyonofx kyonofx commented Jul 20, 2024

Remove the detaches in obtaining rotation matrices and wignerD matrices for correct gradients of model outputs with regard to atomic positions.

Naively remove the detaches result in instability due to gradient explosion for y-axis rotation of ~0 or ~180 degree.

In this fix, we instead do not rotate / rotate 180 degree for edges that are already aligned with the y-axis. This fix enables correct gradients of model outputs with regard to the input atom positions and thus enable training energy-conserving gradient-based force EquiformerV2 models.

@kyonofx kyonofx requested review from wood-b and yilunliao July 20, 2024 00:04
Copy link

codecov bot commented Jul 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
...fairchem/core/models/equiformer_v2/edge_rot_mat.py 97.72% <100.00%> (+0.50%) ⬆️
src/fairchem/core/models/equiformer_v2/so3.py 79.59% <100.00%> (+0.05%) ⬆️

... and 1 file with indirect coverage changes

@kyonofx kyonofx requested a review from clz55 July 20, 2024 00:42
@kyonofx kyonofx requested review from zulissi and lbluque July 22, 2024 17:47
Copy link
Collaborator

@mshuaibii mshuaibii left a comment

Choose a reason for hiding this comment

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

Can you run validation on Val-ID-30k using the pre-trained 31M checkpoint and share the results here. Would be good to make sure things are consistent.

lbluque
lbluque previously approved these changes Jul 22, 2024
Copy link
Collaborator

@lbluque lbluque left a comment

Choose a reason for hiding this comment

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

lgtm.

Agreed with @mshuaibii that making sure direct force models are consistent. In addition it might be worth to write a simple unit-test to make sure the model is indeed properly differentiable.

Comment on lines +57 to +65
output = torch.zeros_like(edge_rot_mat)
mask = (yprod > -0.9999) & (yprod < 0.9999)
output[mask] = edge_rot_mat[mask]
output[~mask, 0, :] = edge_rot_mat[~mask, 0, :]
output[~mask, 2, :] = edge_rot_mat[~mask, 2, :]
output[yprod > 0.9999, 1, :] = edge_rot_mat.new_tensor([[0., 1., 0.]])
output[yprod < -0.9999, 1, :] = edge_rot_mat.new_tensor([[0., -1., 0.]])

return output
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, can this section be replaced with?

edge_rot_mat[yprod > 0.9999, 1, :] = edge_rot_mat.new_tensor([[0., 1., 0.]])
edge_rot_mat[yprod < -0.9999, 1, :] = edge_rot_mat.new_tensor([[0., -1., 0.]])

return edge_rot_mat

@kyonofx kyonofx marked this pull request as draft July 22, 2024 22:47
Copy link

This PR has been marked as stale because it has been open for 30 days with no activity.

@kyonofx
Copy link
Collaborator Author

kyonofx commented Sep 17, 2024

Egrad models will be in dev-mode for now.

@kyonofx kyonofx closed this Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants