Skip to content

Refactor: simplify Mixing DM - #7844

Open
linpeize wants to merge 2 commits into
deepmodeling:LTSfrom
linpeize:mixing_DM_simplify_LTS
Open

Refactor: simplify Mixing DM#7844
linpeize wants to merge 2 commits into
deepmodeling:LTSfrom
linpeize:mixing_DM_simplify_LTS

Conversation

@linpeize

Copy link
Copy Markdown
  1. Refactor: simplify Mixing DM
  2. Fix mixing_beta when exx_separate_loop==true

same as #7462

@mohanchen mohanchen added Refactor Refactor ABACUS codes Long-Time Support (LTS) Issues related to LTS version labels Aug 21, 2026

@dyzheng dyzheng 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.

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:

  1. 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).
  2. 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
linpeize force-pushed the mixing_DM_simplify_LTS branch from 9744b8d to 9056198 Compare August 23, 2026 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Long-Time Support (LTS) Issues related to LTS version Refactor Refactor ABACUS codes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants