Refactor: simplify Mixing DM - #7844
Open
linpeize wants to merge 2 commits into
Open
Conversation
2. add OpenMP in RI_2D_Comm::split_m2D_ktoR()
dyzheng
requested changes
Aug 23, 2026
dyzheng
left a comment
Collaborator
There was a problem hiding this comment.
Summary: This PR is labeled a refactor, but its real payload is a bug fix: on LTS, mixing_beta_for_loop1 is silently ignored when exx_separate_loop == true. The old path calls set_mixing(nullptr), and Mix_Matrix::mix() then creates Plain_Mixing with the default mixing_beta = 1.0 — no caller ever invokes set_mixing_beta. The new set_mixing_plain(mixing_beta_for_loop1) fixes this correctly. That fix alone justifies an LTS backport.
Blockers before merge:
- Rebase needed. Commit f5ea635 duplicates #7777, which was merged into LTS today (ee763df). The current diff's changes to xc_functional_libxc_vxc.cpp and RI_2D_Comm.hpp (230 lines) are entirely that commit. After rebasing onto current LTS, the diff should shrink to roughly #7462's content (+190/−490).
- Double-free risk in Mix_DMk_2D. The class defines a destructor that deletes mixing when flag_del_mixing is set, but copy ctor/assignment are implicitly defined — any copy double-frees. Per repo guidelines (RAII preferred over raw new/delete), the owned case should be a std::unique_ptr<Base_Mixing::Mixing>, with the borrowed case kept as a raw observer pointer. At minimum, = delete the copy operations.
Suggestions:
- Clarify in the PR description why the full refactor (deleting Mix_Matrix, templating Mix_DMk_2D) is backported instead of a minimal fix passing mixing_beta_for_loop1 into the old path. "Keep LTS structurally aligned with develop for future cherry-picks" is a fine rationale — it just needs to be stated.
- Request review from the module owner; currently 0 reviewers.
Non-blocking (belongs to #7777, already merged): guard #include <omp.h> with #ifdef _OPENMP, and check the per-element omp_set_lock overhead in the gamma path.
Tests look fine: removed tests track the deleted class, the new shared-Broyden case is correct, and set_mixing_plain(1.0) expecting output = latest input matches the semantics.
Verdict: Right direction, wrong shape — rebase, fix the copy-safety issue, expand the description, then good to merge.
linpeize
force-pushed
the
mixing_DM_simplify_LTS
branch
from
August 23, 2026 18:36
9744b8d to
9056198
Compare
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.
same as #7462