Repository navigation
Conversation
dyzheng
approved these changes
Oct 12, 2026
Collaborator
|
there are still some issues I need to consider |
mohanchen
reviewed
Oct 12, 2026
7 tasks done
AsTonyshment
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the improvement. However, I still have a couple of concerns noted inline.
AsTonyshment
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the updates. However, I think nowadays AI agents tend to overengineer solutions (e.g., long, repetitive tests), so we should weigh the added complexity against its actual benefit.
To clarify my main points:
- I still think modestly relaxing the main tolerance, for example to twice its current value, would be simpler and more robust than adding a separate fallback that seems unlikely to accept a result (and hard to maintain!!).
- I would also suggest trimming the tests to remove redundant cases.
Collaborator
|
Thanks! Now it looks much cleaner! |
AsTonyshment
approved these changes
Oct 13, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reminder
AGENTS.mdanddocs/developers_guide/agent_governance.md.Linked Issue
No separate issue; numerical reproductions are included in the tests. Base:
457d9dd4a4d1715d4d0c772a77d33f08d7e36ca9.What's changed?
The smeared-occupation Fermi search could return after exhausting its iterations without reaching the requested electron number. Its fixed
1e-10charge tolerance also limited higher-precision calculations, andgweightsused different band counts for searching and writing occupations.This PR uses the explicit band count consistently, selected-spin eigenvalue bounds, compensated occupation sums, and centered Fermi coordinates retained when evaluating occupations and entropy. Gaussian/MP and cold-smearing bases use
erfcto avoid lower-tail cancellation. The public absoluteefremains rounded to one double.Following review, the convergence rule is now simply:
All runtime error-estimation code, its extra MPI reductions and the separate stagnation fallback have been removed. Unresolved stagnation, invalid inputs, unbracketable targets and exhaustion of the named
maxiter = 300remain failures. The factor 16 is a validated allowance, not a universal error bound for arbitrary spectra or MPI layouts.Tests now retain one case per distinct failure mechanism: the duplicate weighted-count and degenerate-shift tests were removed, repeated shifts were trimmed, and scalar references were reduced to six cases covering tails, signed MP values and overshoot.
LowFillingDegenerateSpectrumdescribes the numerical case and covers both the 74-level Gaussian reproduction and a 26-level MP1 cancellation case. The unchangedElecStateTest.CalculateWeightsGWeightsstill covers the original 100-Ry CI regression.Unit Tests and/or Case Tests for my changes
Fresh validation with
OMP_NUM_THREADS=1:MODULE_ESTATEgroupThe serial sweep covers five smearings, 1/3 weighted k points, 1/26/74/256 bands, 5%/30%/90% filling, widths 0.01/1e-8, energy shifts 0/100/1e8, and degenerate/dispersed spectra, plus the two low-filling regressions. On macOS, the 8-epsilon MP1 case stalls at a computed residual of
2.387e-15against1.776e-15; 16 epsilon resolves it without a fallback.Independent 80-digit summation of the returned double occupations gives a maximum residual of 16.114 epsilon times max(1, |N|) across both platforms. Independent 80-digit evaluation of the analytic smearing functions in the saved centered coordinates gives 19.672 epsilon times max(1, |N|). These distinguish the computed stopping criterion from function-evaluation/accumulation error. The independent-sum tests allow one extra epsilon; the external analytic-reference check uses a preselected 20-epsilon limit. Neither margin changes the solver threshold.
The narrow-width/large-offset test's old
2e-15assertion was updated to the new16*epsiloncontract: its observed residual is3.053e-15, below3.553e-15. The subnormal-width failure test remains, with the removed budget text assertion deleted. Other analytic occupation/entropy checks retain their existing limits.Commands and validation scope
The existing Sai build configuration was reused: Release, GNU 13.3, OpenMPI 5.0.10, CMake 3.31.6, CUDA sm70, LCAO/ELPA/Libxc and testing enabled. Changed sources were rebuilt and SHA256-checked against the submitted files.
MPI probes include the current occupation implementation, link the real pool/reduction wrappers, and independently evaluate closed-form occupations in long double. All layouts pass; their maximum reference residual is 19.663 epsilon times N. These are single-node correctness probes with oversubscription, not performance or multi-node results. The first 56-rank probe, linked to the full CUDA-enabled test dependencies, exceeded the allocation's 15-GiB limit; the successful probe links only the required occupation/MPI sources and uses CPU transports. No product change was needed for that harness failure.
Limits: no new full SCF/force/D campaign or guarantee for arbitrarily ill-conditioned spectra, other libm implementations or larger MPI deployments. CUDA-enabled build/test success is not CPU/GPU physical-accuracy validation. External sweeps/probes are validation artifacts, not additional production code or repetitive committed tests.
Governance Notes