fix(rohf): handle linearly dependent bases where n_MO < n_AO (#543) - #554
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a ROHF crash that occurred when linear-dependency removal yields a rectangular MO coefficient matrix (n_MO < n_AO). It updates the ROHF effective-Fock back-transform to handle both square and rectangular coefficient matrices, aligning ROHF behavior with RHF/UHF in reduced MO spaces, and adds regression coverage for GitHub issue #543.
Changes:
- Update the ROHF effective Fock back-transform to branch between the existing square-matrix LAPACK solve (
nAO == nMO) and an overlap-mediated projection for the rectangular case (nMO < nAO). - Thread the AO overlap matrix from
SCFAlgorithminto the ROHF DIIS build path to support the new rectangular transform. - Add C++ and Python regression tests (plus changelog entry) using the issue #543 reproducer system/basis.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
cpp/src/qdk/chemistry/algorithms/microsoft/scf/src/scf_algorithm/diis.cpp |
Implements the new two-branch ROHF back-transform and updates the DIIS ROHF build plumbing. |
cpp/src/qdk/chemistry/algorithms/microsoft/scf/src/scf_algorithm/diis.h |
Updates the build_rohf_f_p_matrix signature and documents the rectangular-case projection behavior. |
cpp/src/qdk/chemistry/algorithms/microsoft/scf/src/scf_algorithm/scf_algorithm.cpp |
Passes the AO overlap matrix into the ROHF DIIS build call during convergence checks. |
cpp/tests/test_scf.cpp |
Adds a regression test covering ROHF with a linearly dependent basis (def2-tzvp) reproducing issue #543. |
python/tests/test_scf.py |
Adds a Python regression test (and structure factory) reproducing issue #543 and asserting ROHF completes. |
docs/source/changelog.rst |
Adds a v2.0.0 WIP bug-fix entry describing the ROHF linear-dependency crash fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
hey Alexis W. Mills (@awoodwa) and David Williams-Young (@wavefunction91) looking forward to your review on this PR , happy to make further changes if needed. |
|
adithyaphanithota (@Adithyaphani) thanks for the contribution! We'll definitely have a look soon. Pinging Boqin Zhang (@BoqinZhang). The only thing I'll note is that this should likely wait until after #453 is merged, as that refactors a large number of ROHF primitives and will likely be a pain to merge directly. |
|
Thanks for taking a look, David Williams-Young (@wavefunction91) — and good catch on #453. It does make sense to hold off. PR #453 moves I'll keep this open and rebase once #453 lands. The core fix here — the overlap-mediated projection Happy to wait on this and make changes if needed. |
|
Hi adithyaphanithota (@Adithyaphani), #453 is merged. |
ac1517d to
01639bb
Compare
|
David Williams-Young (@wavefunction91) Rebased cleanly on top of #453 and #545. The fix is now applied in its correct location — What changed:
The C++ regression test ( Looking forward to your response and happy to make further changes if needed. |
|
David Williams-Young (@wavefunction91) looking forward to your review and happy to make further changes if needed. |
|
Boqin Zhang (@BoqinZhang) and David Williams-Young (@wavefunction91) looking forward to your review and happy to make further changes if needed. |
David Williams-Young (wavefunction91)
left a comment
There was a problem hiding this comment.
This is a very nice fix, thanks for the contribution!
|
David Williams-Young (@wavefunction91) and Boqin Zhang (@BoqinZhang) addressed all maintainer review comments in the latest commit:
Note: the reference energy |
|
David Williams-Young (@wavefunction91) looking forward to your review and happy to make further changes if needed. |
|
Hey adithyaphanithota (@Adithyaphani), thanks for the continued iteration. Can you confirm whether the unit tests pass on your machine? |
…issing overlap factor
7405875 to
e18f615
Compare
|
Hey David Williams-Young (@wavefunction91) — yes, both tests pass locally. David Williams-Young (@wavefunction91) looking forward to your response . |
|
David Williams-Young (@wavefunction91) and Boqin Zhang (@BoqinZhang) looking forward to you review . |
|
Hey David Williams-Young (@wavefunction91) and Boqin Zhang (@BoqinZhang) looking forward to you review . Are you'll looking to further to proceed with this issue fix or not . |
|
|
||
| // Always validate the basic contract from the issue report. | ||
| EXPECT_TRUE(orbitals->is_restricted()); | ||
| EXPECT_TRUE(std::isfinite(energy)); |
There was a problem hiding this comment.
Hi adithyaphanithota (@Adithyaphani) , is it possible for you to add your reference energy at here? It would be very helpful for us to make the comparison. Thank you!
There was a problem hiding this comment.
Hi Boqin Zhang (@BoqinZhang), happy to add it! The reference energy was omitted because the test skips on most platforms when linear-dependency removal doesn't fire. Could you share the converged ROHF/def2-tzvp energy from your infrastructure? We'll add the EXPECT_NEAR right away.
| orbitals = wavefunction.get_orbitals() | ||
| # Always validate the basic contract from the issue report. | ||
| assert orbitals.is_restricted() | ||
| assert np.isfinite(energy) |
There was a problem hiding this comment.
Hi adithyaphanithota (@Adithyaphani) , Like the cpp test, is it possible for you to add your reference energy at here? It would be helpful for us to make a comparison. Thank you!
There was a problem hiding this comment.
Boqin Zhang (@BoqinZhang) Same situation here — happy to add np.isclose once we have a confirmed converged value. Could you run the o-benzosemiquinone ROHF/def2-tzvp calculation on your end and share the energy? We'll update both tests immediately.
|
Boqin Zhang (@BoqinZhang) looking forward to your response. |
|
David Williams-Young (@wavefunction91) and Boqin Zhang (@BoqinZhang) it's almost been a week looking forward to your response on this PR. |
Fixes #543
Problem
ROHF with
scf_type="restricted"crashes on any system where the AO basis is linearly dependent. When a basis set is linearly dependent on a given geometry (e.g.def2-tzvpon planar o-benzosemiquinone), the solver drops redundant basis functions so thatn_MO < n_AO. The MO coefficient matrixCbecomes rectangular (nAO × nMO) and the existing code path fails.Root cause
SCFAlgorithm::build_rohf_f_p_matrixinscf_algorithm.cpphard-assertednAO == nMOand used a LAPACKgetrf/getrssquare-matrix inversion to back-transformF_eff_MO → F_eff_AO. A rectangularCmakes that inversion undefined.Fix
Two-branch strategy in
build_rohf_f_p_matrix:nAO == nMOC⁻ᵀ F_MO C⁻¹— unchangednMO < nAOF_eff_AO = S C F_MO_eff Cᵀ Sviasimilarity_transformThe projection satisfies
Cᵀ F_eff_AO C = F_MO_effwheneverCᵀ S C = I(standard MO orthonormality w.r.t.S). This is identical to how RHF and UHF operate in the reduced MO space after linear-dependency removal.Additional guards: direction-only assertion (
nMO > nAOstill throws), S dimension validation, and electron count validation (nelec > nMOthrows with a clear message).Files changed
scf_algorithm.hbuild_rohf_f_p_matrixsignature + docstring (addsSparam)scf_algorithm.cppscf_impl.overlap()at call sitecpp/tests/ut_common.hppcreate_obenzosemiquinone_structure()molecule generatorcpp/tests/test_scf.cppROHF_RectangularBackTransform_ProjectionIdentity(deterministic) andROHF_LinearlyDependentBasis_Issue543(integration)python/tests/test_scf.pycreate_obenzosemiquinone_structure()andtest_rohf_linearly_dependent_basis_issue_543Testing
ROHF_RectangularBackTransform_ProjectionIdentitydirectly verifiesCᵀ F_eff_AO C = F_MO_effwith a synthetic 4×2 non-identity S and S-orthonormal C — no SCF run needed.ROHF_LinearlyDependentBasis_Issue543uses the exact reproducer from the issue (o-benzosemiquinone,def2-tzvp, doublet,scf_type="restricted") and asserts no crash, finite energy, and restricted orbitals. Skips with a clear message on platforms where linear-dependency removal doesn't fire or drops more functions than electrons.