Skip to content

Fix: Include index zero in nspin=4 G=0 magnetization contributions - #8146

Open
AsTonyshment wants to merge 1 commit into
deepmodeling:developfrom
AsTonyshment:fix/nspin4_ig0
Open

AsTonyshment wants to merge 1 commit into
deepmodeling:developfrom
AsTonyshment:fix/nspin4_ig0

Conversation

@AsTonyshment

Copy link
Copy Markdown
Collaborator

What's changed?

Change three ig0 > 0 checks to ig0 >= 0 so that the nspin=4 convergence metric and traditional/angle mixing inner products include the $G=0$ magnetization contribution when its local index is zero.

PS: All integration tests passed with reference values unchanged, suggesting that the defect is inactive in the tested layouts, although it can still be triggered when the $G=0$ component occupies local index zero.

@ABACUS-PR-BOT ABACUS-PR-BOT left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Static review of 8afdd56dc0919eebe51a95d6619f03ba79d00246, including independent verification: no introduced correctness defect found. git diff --check passed; the trusted governance check reported warnings only. Builds, runtime tests and CI were not verified.

Non-blocking: Please record the exact integration-test command/configuration and observed result in the PR description, and add a brief rationale that no INPUT documentation update is needed for this correction to the existing metric.

if (ig0 > 0)
// Taoni fix 2026-10-14
// Index zero is valid; a negative index means this rank does not own G=0.
if (ig0 >= 0)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Non-blocking: Please add an explicit ig_gge0 == 0 regression with a zero charge residual and nonzero G=0 magnetization, covering the convergence metric and both traditional/angle mixing metrics. The existing test_chg_mix.cpp fixture places G=0 at index 2, and test_chg_drho_inner.cpp only tests nspin=1, so those tests do not distinguish > 0 from >= 0. Check the expected nonzero contribution and include a -1 no-local-G0 case.

@AsTonyshment AsTonyshment Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Maybe you are right, but to me personally I think this is a bit of over-engineering. I think no one will change it back to ig0 > 0, especially in the age of agent-assisted programming...

This branch has not been deployed

No deployments
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.

2 participants