Conversation
Need access to hbar expression sometimes. Also makes sense to centralize the hbar construction.
d3d762f to
c04d784
Compare
evaleev
left a comment
There was a problem hiding this comment.
Review
Good refactoring — centralizing H̄ construction into CC::hbar() significantly reduces duplication and the API addition is clean. A few items to address:
1. Semantic change in CC::λ() when hbar_comm_rank_ is explicitly set
Old code:
const auto commutator_rank = hbar_comm_rank_.value_or(4);
auto hbar = mbpt::lst(H(), T(N, skip_singles), commutator_rank - 1);New code:
const auto commutator_rank = hbar_comm_rank_.value_or(3);
auto hbar = this->hbar(commutator_rank);The default path is preserved (old: 4−1=3, new: 3). But when hbar_comm_rank_ is explicitly set — e.g. to 5 — the old code used 5−1=4, while the new code uses 5 directly. The commit says "fixup truncation logic" so this may be intentional, but the rationale isn't documented. Is the old −1 a bug being fixed, or was it deliberate (λ equations needing one fewer commutator order because the derivative of H̄ w.r.t. T reduces the rank by one)?
Please clarify. If the old −1 was correct, this is a regression for callers that set hbar_comm_rank explicitly.
2. Subtle behavior change in tʼ() for orbital-optimized ansatz
Old [hbar, T'(1)] construction used T(N) (includes singles); the new this->hbar() uses T(N, skip_singles). For Ansatz::oT this now skips singles, which is arguably more correct but is a behavioral change. Similarly in λʼ() at the hbar_pert line — guarded by an ansatz_ == Ansatz::T assert today, but would matter if that assert is relaxed.
Minor
- Doxygen (
cc.hpp:75): "will use the value ofhbar_comm_rank" — should referenceOptions::hbar_comm_rankor the member for clarity. - Commit message typo: "fixup truncation login" → "logic".
|
This PR is ready to go, all issues have been addressed. |
Allow users to skip singles even for non-orbital-optimized ansätze by adding `skip_singles` to `CC::Options`. Defaults to true for oT/oU and false for T/U; asserts that orbital-optimized ansätze cannot disable it.
Need easy access to hbar expression sometimes