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.

Sign up for GitHub

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

Jump to bottom

fix: make the se attn v2 descriptor energy conservative. #2905

Merged
merged 15 commits into from
Oct 11, 2023
Merged

fix: make the se attn v2 descriptor energy conservative. #2905

merged 15 commits into from
Oct 11, 2023

Conversation

wanghan-iapcm
Copy link
Collaborator

This PR fixes issue #2811

  1. fix the auto-diff issue: input should be descriptor_reshape.
  2. fix the discontinuity introduced in the attention map.

@wanghan-iapcm wanghan-iapcm requested a review from iProzd October 9, 2023 11:31
@wanghan-iapcm wanghan-iapcm linked an issue Oct 9, 2023 that may be closed by this pull request
Energy drift in NVE MD with potential generated with "se_atten" descriptor #2811
Closed
@wanghan-iapcm wanghan-iapcm changed the base branch from master to devel October 9, 2023 11:33
@codecov
Copy link

codecov bot commented Oct 9, 2023 •
edited
Loading

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (d8ee74b) 75.37% compared to head (5559d2a) 75.77%.
Report is 2 commits behind head on devel.

Additional details and impacted files 
@@            Coverage Diff             @@
##            devel    #2905      +/-   ##
==========================================
+ Coverage   75.37%   75.77%   +0.39%     
==========================================
  Files         245      246       +1     
  Lines       24635    25048     +413     
  Branches     1581     1618      +37     
==========================================
+ Hits        18568    18979     +411     
+ Misses       5137     5104      -33     
- Partials      930      965      +35
Files Coverage Δ
deepmd/descriptor/se_atten.py 93.92% <100.00%> (+0.08%) ⬆️
deepmd/op/_tabulate_grad.py 100.00% <100.00%> (ø)
source/lib/src/tabulate.cc 57.89% <50.00%> (-0.15%) ⬇️
source/op/tabulate_multi_device.cc 54.65% <53.33%> (+0.35%) ⬆️

... and 17 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

njzjz
njzjz requested changes Oct 9, 2023
View reviewed changes
Copy link
Member

@njzjz njzjz left a comment

Choose a reason for hiding this comment

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

It seems to be a critical bug that the energy does not match the force. Do we need to add it to #2866?

deepmd/descriptor/se_atten.py Outdated Show resolved Hide resolved
@njzjz njzjz requested a review from nahso October 9, 2023 20:19
@njzjz
implement dy_dtwo
63a4aee 
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
@njzjz njzjz added the Test CUDA Trigger test CUDA workflow label Oct 9, 2023
@github-actions github-actions bot removed the Test CUDA Trigger test CUDA workflow label Oct 9, 2023
@njzjz njzjz added the Test CUDA Trigger test CUDA workflow label Oct 9, 2023
@github-actions github-actions bot removed the Test CUDA Trigger test CUDA workflow label Oct 9, 2023
@njzjz njzjz added the Test CUDA Trigger test CUDA workflow label Oct 9, 2023
@github-actions github-actions bot removed the Test CUDA Trigger test CUDA workflow label Oct 9, 2023
@njzjz njzjz added the Test CUDA Trigger test CUDA workflow label Oct 9, 2023
@github-actions github-actions bot removed the Test CUDA Trigger test CUDA workflow label Oct 9, 2023
njzjz and others added 2 commits October 9, 2023 20:13
@njzjz
Update source/lib/tests/test_tabulate_se_a.cc
99fe74d 
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
f2bf0cd 
for more information, see https://pre-commit.ci
@njzjz njzjz added the Test CUDA Trigger test CUDA workflow label Oct 10, 2023
@github-actions github-actions bot removed the Test CUDA Trigger test CUDA workflow label Oct 10, 2023
@njzjz njzjz mentioned this pull request Oct 10, 2023
Open
@njzjz njzjz added the Test CUDA Trigger test CUDA workflow label Oct 11, 2023
nahso
nahso approved these changes Oct 11, 2023
View reviewed changes
source/lib/src/gpu/tabulate.cu Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Test CUDA Trigger test CUDA workflow label Oct 11, 2023
@wanghan-iapcm wanghan-iapcm merged commit b426d14 into deepmodeling:devel Oct 11, 2023
@wanghan-iapcm wanghan-iapcm deleted the smooth-se-attn-v2 branch October 11, 2023 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nahso nahso nahso approved these changes

@iProzd iProzd iProzd approved these changes

@njzjz njzjz njzjz approved these changes

Assignees
No one assigned
Labels
Core CUDA OP Python ROCM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Energy drift in NVE MD with potential generated with "se_atten" descriptor
4 participants
@wanghan-iapcm @njzjz @nahso @iProzd