Skip to content

Introduce CC::hbar#509

Merged
evaleev merged 7 commits intomasterfrom
ajay/feat/cc-hbar
Mar 26, 2026
Merged

Introduce CC::hbar#509
evaleev merged 7 commits intomasterfrom
ajay/feat/cc-hbar

Conversation

@ajay-mk
Copy link
Copy Markdown
Member

@ajay-mk ajay-mk commented Mar 20, 2026

Need easy access to hbar expression sometimes

ajay-mk added 2 commits March 17, 2026 18:21
Need access to hbar expression sometimes. Also makes sense to centralize the hbar construction.
@ajay-mk ajay-mk force-pushed the ajay/feat/cc-hbar branch from d3d762f to c04d784 Compare March 23, 2026 06:45
@ajay-mk ajay-mk marked this pull request as ready for review March 23, 2026 11:49
@ajay-mk ajay-mk added the feature New feature label Mar 23, 2026
@ajay-mk ajay-mk added this to the 2.3 milestone Mar 23, 2026
@ajay-mk ajay-mk requested a review from evaleev March 23, 2026 14:32
Copy link
Copy Markdown
Member

@evaleev evaleev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 of hbar_comm_rank" — should reference Options::hbar_comm_rank or the member for clarity.
  • Commit message typo: "fixup truncation login" → "logic".

@ajay-mk
Copy link
Copy Markdown
Member Author

ajay-mk commented Mar 25, 2026

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.
@evaleev evaleev merged commit ae2f995 into master Mar 26, 2026
16 checks passed
@evaleev evaleev deleted the ajay/feat/cc-hbar branch March 26, 2026 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants