Fix: refresh v_eff again after updating XC type - #7840
Open
maki49 wants to merge 2 commits into
Open
Conversation
…enpairs in the 1st iter of the EXX loop.
There was a problem hiding this comment.
Pull request overview
Fixes Issue #7839 by ensuring v_eff is refreshed immediately after the XC functional switches from the first (PBE/GGA) loop to the hybrid/EXX loop, preventing the first EXX iteration from building a Hamiltonian with a stale GGA v_eff and then adding EXX on top (double-counting exchange).
Changes:
- In the LRI EXX path, detect the
two_level_step0→1 transition afterexx_after_converge()and force a potential refresh viaElecState::cal_converged(). - In the LIP solver EXX path, apply the same refresh on the 0→1 transition in
iter_finish()to avoid carrying stalev_effinto the first EXX iteration. - Add explanatory comments documenting why the refresh is required at the transition point.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| source/source_lcao/module_ri/exx_lri_interface.hpp | Refreshes v_eff via cal_converged() on the 0→1 EXX two-level transition after switching XC type. |
| source/source_esolver/esolver_ks_lcaopw.cpp | Applies the same v_eff refresh on the solver-side 0→1 transition for the LIP EXX workflow. |
Suppressed comments (1)
source/source_esolver/esolver_ks_lcaopw.cpp:236
- Rule: Add focused tests for key features, bug fixes, INPUT behavior changes, heterogeneous kernels, and core-module refactors.
Severity: warning
Location: source/source_esolver/esolver_ks_lcaopw.cpp:233-236
Reason: This change fixes a two-level EXX/hybrid transition bug (Issue #7839) but the PR does not add a regression test, despite the repository having C++ test infrastructure (e.g.source/source_esolver/test/andsource/source_lcao/module_ri/test/).
Suggested action: Add a focused regression test that exercises the 0->1 XC switch and verifies the first EXX-iteration Hamiltonian/eigenvalues are not polluted by a stale GGA v_eff (e.g. the reported H2 SZ HF case or a smaller unit-level check around the XC switch + potential refresh).
Exception: human approval required
if (!conv_esolver && two_level_step_before == 0 && this->two_level_step == 1)
{
this->pelec->cal_converged();
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
to prevent adding an extra PBE potential onto the hybrid Hamiltonian in the 1st iter of the EXX loop
Fix #7839