Fix DMR spin-channel mismatch in openshell LR spectra - #8016
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Add focused regression tests and remove the ambiguous default argument.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Fixes spin-channel mixing in open-shell, multi-k length-form LR spectra and adds MPI-safe HContainer handling.
Changes:
- Adds spin-selective complex DMR extraction.
- Passes the active spin channel during spectrum calculation.
- Guards unavailable distributed HContainer pairs.
| File | Summary |
|---|---|
source/source_lcao/module_lr/utils/lr_util_hcontainer.h |
Declares spin-selective extraction; the new type default should be removed. |
source/source_lcao/module_lr/utils/lr_util_hcontainer.cpp |
Implements channel extraction and MPI pair checks; focused regression coverage is missing. |
source/source_lcao/module_lr/lr_spectrum.cpp |
Uses the selected spin channel; the bug-specific regression case is not covered. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
LR_Spectrum<complex<double>>::cal_transition_dipole_istate_length looped over all nspin_x spin channels of the transition density matrix but called get_DMR_real_imag_part() without selecting a channel, so the function copied every channel of DM_trans (size nspin_x) into the single-channel DM_trans_real_imag. For nspin_x==2 (open-shell/spin- polarized, multi-k) this hits an assertion failure (or out-of-bounds access in release builds), and even when it doesn't crash it double- counts the merged density on each iteration of the outer loop. Add an overload of get_DMR_real_imag_part() that copies a single named spin channel into the (always single-channel) DMR_real, and use it from the two call sites in cal_transition_dipole_istate_length so each outer loop iteration only processes its own channel, matching the double (gamma-only) specialization's behavior. Verified: reproduced the original assertion failure with gdb on the open-shell (nspin=2) multi-k length-gauge path, confirmed the fix resolves it, and confirmed the fixed multi-k oscillator strength / transition dipoles match the gamma-only result exactly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
get_DMR_real_imag_part() (both overloads) and set_HR_real_imag_part() dereferenced the result of HContainer::find_pair(ia, ja) without a null check. Under real multi-rank MPI parallelism, the HContainer is distributed 2D block-cyclic, so find_pair() legitimately returns nullptr for atom pairs not owned by the current rank — dereferencing that pointer segfaults. Reproduced with gdb on tests/08_EXX/54_GO_ULR_HF (gamma_only=0, KPT 1 1 1, mpirun -np 2): SIGSEGV in get_DMR_real_imag_part, called from OperatorLRHxc::grid_calculation during the Casida eigenvalue solve — a different call site/crash than the spin-channel-mismatch bug fixed in the previous commit, and only reproducible with more than one MPI rank (a single-rank run owns every atom pair locally, masking the bug). Skip atom pairs not present on the local rank, matching the existing pattern used elsewhere in the codebase for MPI-parallel HContainer access. Verified: the same 54_GO_ULR_HF case now runs cleanly under mpirun -np 2 (gdb shows a clean exit, no crash) and its excitation energies exactly match both the gamma_only=1 run and result.ref. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Upstream deepmodeling#8000 renamed elecstate::DensityMatrix to module_dm::DensityMatrix and get_DMR_vector/get_DMR_pointer to get_dmr_vec/get_dmr_ptr; update the spin-index overload added in this branch accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mohanchen
approved these changes
Sep 25, 2026
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.
The bug only happens in: openshell LR (lr_unrestricted=1) && length form (abs_gauge="length") && multi-k (gamma_only=0).
DMR will be doubled before the fix. Now it gives the same oscillator strength as the gamma-only case.