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

Regression test for Ewald stress tensor #1375

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bhourahine
Copy link
Member

Geometry is not relaxed, so tensor has finite values for all elements.

Geometry is not relaxed, so tensor has finite values for all elements.
@bhourahine
Copy link
Member Author

@terminationshock hopefully this should be sensitive to the your test changes in the Ewald stress, so can be a valid regression test for refactoring that routine.
While looking at this, I was reminded that the reciprocal Ewald for 1/r and 1/r2 do not use the distributeRangeInChunks() mechanism. Do you think that would also help with the timings for the Ewald use case that started this discussion of vectorising?

Copy link

codecov bot commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c72a10e) 70.63% compared to head (53d845b) 70.63%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1375   +/-   ##
=======================================
  Coverage   70.63%   70.63%           
=======================================
  Files         230      230           
  Lines       43943    43943           
=======================================
  Hits        31041    31041           
  Misses      12902    12902           

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

@terminationshock
Copy link
Contributor

Your new test is not sensitive to changes in derivStressEwaldRec. My diff is:

diff --git a/src/dftbp/dftb/coulomb.F90 b/src/dftbp/dftb/coulomb.F90
index d864717b..152292ba 100644
--- a/src/dftbp/dftb/coulomb.F90
+++ b/src/dftbp/dftb/coulomb.F90
@@ -2158,8 +2158,8 @@ contains
       sigma(:,:) = sigma + (-unity + sTmp * spread(gg, 1, 3)*spread(gg, 2, 3)) * cos(rg) * eTerm
     end do
     ! note factor of 2 as only summing over half of reciprocal space
-    recSum(:) = 2.0_dp * recSum * 4.0_dp * pi / vol
-    sigma(:,:) = 2.0_dp * sigma * 4.0_dp * pi / vol
+    recSum(:) = 1.0_dp * recSum * 4.0_dp * pi / vol
+    sigma(:,:) = 1.0_dp * sigma * 4.0_dp * pi / vol
 
   end subroutine derivStressEwaldRec

The test is still successful, though:

Test #14: dftb+_scc/GaAs_8 ....................   Passed

@bhourahine bhourahine marked this pull request as draft May 16, 2024 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants