Skip to content

Fix acoustic_sum_rule computation in PhononBSDOSDoc#1447

Merged
esoteric-ephemera merged 7 commits into
materialsproject:developfrom
hrushikesh-s:fix/acoustic-sum-rule-einsum
May 8, 2026
Merged

Fix acoustic_sum_rule computation in PhononBSDOSDoc#1447
esoteric-ephemera merged 7 commits into
materialsproject:developfrom
hrushikesh-s:fix/acoustic-sum-rule-einsum

Conversation

@hrushikesh-s
Copy link
Copy Markdown
Contributor

The previous einsum('iijk->jk', fcs) summed only the on-site (i==j) self-interaction blocks, which is unrelated to the acoustic sum rule. The ASR condition is sum_j Phi_ij = 0 for every atom i.

Compute per-atom row sums and return the one with the largest Frobenius norm, so the field reports the worst per-atom violation.

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • Replace the on-site einsum with a per-atom row sum
  • I have run the tests locally and they passed.
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 7, 2026

Codecov Report

❌ Patch coverage is 11.53846% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.56%. Comparing base (df5dc60) to head (0e5af9d).

Files with missing lines Patch % Lines
emmet-core/emmet/core/phonon.py 11.53% 23 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1447      +/-   ##
===========================================
- Coverage    85.66%   85.56%   -0.10%     
===========================================
  Files          239      239              
  Lines        19167    19183      +16     
===========================================
- Hits         16419    16414       -5     
- Misses        2748     2769      +21     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tsmathis tsmathis changed the base branch from main to develop May 7, 2026 23:21
@tsmathis tsmathis requested a review from esoteric-ephemera May 7, 2026 23:21
@tsmathis
Copy link
Copy Markdown
Collaborator

tsmathis commented May 7, 2026

seems okay to me, but I don't have the domain knowledge (Aaron, thoughts?)

@esoteric-ephemera
Copy link
Copy Markdown
Collaborator

@hrushikesh-s looking at ex this review, and using the notation that the ($q=0$) IFCs are N (# of atoms) x N x 3 x 3 tensors $C_{ij \alpha\beta}$, $i,j=1,2,...,N$ and $\alpha,\beta \in [1,2,3]$,

$$ \sum_j C_{ij \alpha\beta} = 0 $$

So perhaps the easiest checks of the ASR are:

np.abs(np.array(self.force_constants).sum(axis=1)).max() < thresh

with thresh some small number. Or even

np.all(np.abs(np.array(self.force_constants).sum(axis=1)) < thresh)

Thoughts?

@hrushikesh-s
Copy link
Copy Markdown
Contributor Author

Thanks @esoteric-ephemera. I have now switched to np.abs(fcs.sum(axis=1)).max().
I have kept it as a raw scalar rather than < thresh so users can pick their own threshold and we keep quantitative info.

This new approach is also verified on mp-149, mp-189, mp-556120 --> all give ~1e-14, which is correct.
As a consequence, all three have the expected linear slopes for all of their acoustic modes near the Gamma-point, which is physically correct.

Looks good?

@tsmathis
Copy link
Copy Markdown
Collaborator

tsmathis commented May 8, 2026

whichever method is decided on, it would be good if a citation/link (doi?) to the corresponding paper could be added that future develops can reference

The previous einsum('iijk->jk', fcs) summed only the on-site (i==j) self-interaction blocks, which is unrelated to the acoustic sum rule. The ASR condition is sum_j Phi_ij = 0 for every atom i.

Compute per-atom row sums and return the one with the largest Frobenius norm, so the field reports the worst per-atom violation.
@esoteric-ephemera esoteric-ephemera force-pushed the fix/acoustic-sum-rule-einsum branch from 3db966f to ed0cdff Compare May 8, 2026 20:14
@esoteric-ephemera
Copy link
Copy Markdown
Collaborator

Yep looks good to me, will add the citation now

@esoteric-ephemera
Copy link
Copy Markdown
Collaborator

@hrushikesh-s: I tweaked this a bit to return the N x 3 x 3 matrix from the ASR, since the same check on $max | \sum_j C_{ij\alpha \beta} |$ is done in check_sum_rule_deviations

@tsmathis removed the computed fields on a lot of these since I don't know that we need to save heavy arrays in memory. Thoughts?

@tsmathis
Copy link
Copy Markdown
Collaborator

tsmathis commented May 8, 2026

I don't know that we need to save heavy arrays in memory

Yep that's reasonable. Merge whenever you're happy 👍
(and thanks for adding the references)

@esoteric-ephemera esoteric-ephemera merged commit 72c5a2e into materialsproject:develop May 8, 2026
17 checks passed
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.

4 participants