Add DUCC to qdk-chemistry

[Agam Shayit (agamshayit-ms)]

Add initial DUCC implementation.
Uses BTAS code to evaluate wick-contracted BCH up to second order

[Copilot]

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (1)

cpp/CMakeLists.txt:187

• target_link_libraries() is using a $<BUILD_INTERFACE:...> generator expression for a target link item. This can stop CMake from treating BTAS::BTAS as a real target dependency (so its include dirs/usage requirements may not propagate), and it isn’t necessary here because PRIVATE deps are not exported anyway. Link to the target directly and keep it PRIVATE to avoid exporting it.

# Header-only and absent from public headers, so keep BTAS out of the export.
target_link_libraries(chemistry PRIVATE $<BUILD_INTERFACE:BTAS::BTAS>)

[Copilot]

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (3)

python/src/qdk_chemistry/plugins/openfermion/conversion.py:72

• The docstring describes the InteractionOperator two-body tensor as "⟨pq|sr⟩" and equates it to (ps|qr) in chemist notation, but the implementation below constructs blocks via transpose(0, 2, 3, 1). As written, the docstring’s index mapping doesn’t match the actual permutation used, which can mislead future maintenance/debugging.

    OpenFermion defines ``h[p,q,r,s]`` via
    ``H₂ = ½ Σ h[p,q,r,s] a†_p a†_q a_r a_s``, which in physicist (Dirac)
    notation is ``⟨pq|sr⟩``, or equivalently ``(ps|qr)`` in chemist notation.

python/src/qdk_chemistry/plugins/openfermion/conversion.py:90

• The inline comment claims the (0, 2, 3, 1) transpose yields h2_so[p,q,r,s] = source[p,s,q,r] / (ps|qr), but source.transpose(0, 2, 3, 1) actually maps to source[p,r,s,q]. This makes the rationale internally inconsistent even if the code is correct.

    # OpenFermion's InteractionOperator sums ``h[p,q,r,s] a+_p a+_q a_r a_s`` and
    # the caller passes ``0.5 * h2_so``.  Matching this to the electronic
    # Hamiltonian ``1/2 sum_pqrs sum_st (pq|rs)_st a+_ps a+_rt a_st a_qs`` (spins
    # s, t) requires each spin-orbital block to be a ``(0, 2, 3, 1)`` transpose of
    # its chemist source:  h2_so[p, q, r, s] = (ps|qr)_chemist = source[p, s, q, r].

cpp/include/qdk/chemistry/algorithms/effective_hamiltonian.hpp:57

• Typo in the Doxygen conditional block tag: DOXYGEN_SUPRESS should be DOXYGEN_SUPPRESS. As-is, Doxygen won’t recognize the directive and may emit warnings for the argument-pack documentation.

   * \cond DOXYGEN_SUPRESS (Doxygen warning suppression for argument packs)

[Copilot]

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (2)

python/src/qdk_chemistry/plugins/openfermion/conversion.py:90

• The explanatory comment claims the transpose implements h2_so[p,q,r,s] = source[p,s,q,r], but transpose(0, 2, 3, 1) actually maps h2_so[p,q,r,s] = source[p,r,s,q]. This mismatch makes the conversion logic hard to audit (especially given the restricted-path transpose just below). Update the comment to describe the axis reorder that the code really applies.

    # OpenFermion's InteractionOperator sums ``h[p,q,r,s] a+_p a+_q a_r a_s`` and
    # the caller passes ``0.5 * h2_so``.  Matching this to the electronic
    # Hamiltonian ``1/2 sum_pqrs sum_st (pq|rs)_st a+_ps a+_rt a_st a_qs`` (spins
    # s, t) requires each spin-orbital block to be a ``(0, 2, 3, 1)`` transpose of
    # its chemist source:  h2_so[p, q, r, s] = (ps|qr)_chemist = source[p, s, q, r].

cpp/CMakeLists.txt:187

• chemistry is created without an explicit type (add_library(chemistry)), so it can be built as STATIC when BUILD_SHARED_LIBS=OFF. In that case, linking BTAS as PRIVATE will not propagate BTAS to downstream link lines, which can produce unresolved symbols if BTAS is not purely header-only. Consider making the BTAS dependency PUBLIC for static builds (or otherwise ensuring downstream targets link BTAS when needed).

# Header-only and absent from public headers, so keep BTAS out of the export.
target_link_libraries(chemistry PRIVATE $<BUILD_INTERFACE:BTAS::BTAS>)

[Copilot]

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (1)

python/src/qdk_chemistry/plugins/openfermion/conversion.py:90

• The explanatory comment for the (0, 2, 3, 1) transpose is internally inconsistent: transpose(0, 2, 3, 1) implies h2_so[p, q, r, s] = source[p, r, s, q], but the comment currently states source[p, s, q, r]. Aligning the comment with the actual mapping will avoid confusion when maintaining the OpenFermion convention logic (especially given the subtle symmetry assumptions discussed here).

    # s, t) requires each spin-orbital block to be a ``(0, 2, 3, 1)`` transpose of
    # its chemist source:  h2_so[p, q, r, s] = (ps|qr)_chemist = source[p, s, q, r].

[Copilot]

Pull request overview

Copilot reviewed 23 out of 24 changed files in this pull request and generated no new comments.

Suppressed comments (5)

python/src/qdk_chemistry/plugins/openfermion/conversion.py:109

• This inline comment describes a different index mapping than what transpose(0, 2, 3, 1) actually assigns. Keeping the comment consistent with the code helps prevent accidental reintroduction of the non-8-fold bug this PR is addressing.

    # alpha-beta block: h2_so[2p, 2q+1, 2r+1, 2s] = h2_aabb[p,s,q,r]
    h2_so[e, o, o, e] = h2_aabb.transpose(0, 2, 3, 1)

cpp/CMakeLists.txt:147

• chemistry is always linked against BTAS::BTAS, but the alias BTAS::BTAS is only created in the FetchContent fallback. If find_package(BTAS) succeeds but provides a non-namespaced target (e.g. BTAS), the build will fail at link time. Ensure BTAS::BTAS exists in both cases before using it.

find_package(BTAS QUIET)
if(NOT BTAS_FOUND)
    include(FetchContent)
    FetchContent_Declare(BTAS
        GIT_REPOSITORY https://github.com/BTAS/btas.git

python/src/qdk_chemistry/plugins/openfermion/conversion.py:90

• The explanatory comment contradicts the actual transpose being applied. transpose(0, 2, 3, 1) produces source[p, r, s, q], but the comment claims source[p, s, q, r] (and also references the wrong prior transpose). This makes the conversion logic hard to audit and risks future regressions.

    # OpenFermion's InteractionOperator sums ``h[p,q,r,s] a+_p a+_q a_r a_s`` and
    # the caller passes ``0.5 * h2_so``.  Matching this to the electronic
    # Hamiltonian ``1/2 sum_pqrs sum_st (pq|rs)_st a+_ps a+_rt a_st a_qs`` (spins
    # s, t) requires each spin-orbital block to be a ``(0, 2, 3, 1)`` transpose of
    # its chemist source:  h2_so[p, q, r, s] = (ps|qr)_chemist = source[p, s, q, r].

docs/source/user/comprehensive/algorithms/index.rst:18

• The toctree lists effective_hamiltonian_constructor twice, which can lead to duplicate entries/warnings in the built docs. Remove the duplicate entry.

   effective_hamiltonian_constructor
   expectation_estimator
   hamiltonian_constructor
   effective_hamiltonian_constructor
   localizer

cpp/src/qdk/chemistry/algorithms/CMakeLists.txt:16

• effective_hamiltonian.cpp is listed twice in target_sources, which can cause duplicate compilation entries/warnings and complicates build debugging. Keep only one entry.

    dynamical_correlation_calculator.cpp
    effective_hamiltonian.cpp
    geometry_optimization.cpp
    effective_hamiltonian.cpp
    scf.cpp

[Copilot]

Pull request overview

Copilot reviewed 24 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (4)

python/src/qdk_chemistry/plugins/openfermion/conversion.py:71

• The docstring states OpenFermion uses ⟨pq|sr⟩ / (ps|qr) for h[p,q,r,s], but the actual conversion in both restricted and unrestricted paths is the (0, 2, 3, 1) transpose (chemist -> physicist ⟨pr|qs⟩). This inconsistency makes it hard to verify the convention and maintain the code.

    OpenFermion defines ``h[p,q,r,s]`` via
    ``H₂ = ½ Σ h[p,q,r,s] a†_p a†_q a_r a_s``, which in physicist (Dirac)
    notation is ``⟨pq|sr⟩``, or equivalently ``(ps|qr)`` in chemist notation.

python/src/qdk_chemistry/plugins/openfermion/conversion.py:92

• These comments describe the (0, 2, 3, 1) transpose but then give an index mapping source[p, s, q, r] that does not match what transpose(0, 2, 3, 1) actually does (it maps to source[p, r, s, q]). Please align the index mapping in the comment with the implemented transpose so future changes don't reintroduce the non-8-fold bug.

    # Hamiltonian ``1/2 sum_pqrs sum_st (pq|rs)_st a+_ps a+_rt a_st a_qs`` (spins
    # s, t) requires each spin-orbital block to be a ``(0, 2, 3, 1)`` transpose of
    # its chemist source:  h2_so[p, q, r, s] = (ps|qr)_chemist = source[p, s, q, r].
    # (The ``(0, 3, 2, 1)`` transpose used previously only coincides with this for
    # integrals with full chemist 8-fold symmetry; it silently corrupts the

python/src/qdk_chemistry/plugins/openfermion/conversion.py:108

• This alpha-beta block comment shows an index mapping (h2_aabb[p,s,q,r]) that doesn't match the implemented transpose(0, 2, 3, 1) (which maps to h2_aabb[p, r, s, q]). Updating the comment avoids future confusion when debugging spin-block conversions.

    # alpha-beta block: h2_so[2p, 2q+1, 2r+1, 2s] = h2_aabb[p,s,q,r]

cpp/src/qdk/chemistry/algorithms/CMakeLists.txt:16

• effective_hamiltonian.cpp is listed twice in target_sources, which is redundant and can lead to confusing build output or duplicate compilation in some generators. Remove the duplicate entry so the source list is unambiguous.

    dynamical_correlation_calculator.cpp
    effective_hamiltonian.cpp
    geometry_optimization.cpp
    effective_hamiltonian.cpp
    scf.cpp

[Copilot]

🟡 Changes recommended

cpp/src/qdk/chemistry/algorithms/CMakeLists.txt lists effective_hamiltonian.cpp twice, which can break the build via duplicate compilation/linking.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 25/26 changed files
• Comments generated: 1
• Review effort level: Lite