Skip to content

Unaddressed review items from merged PR #760 (mixed real/complex BlockUniTensor contract) #855

@IvanaGyro

Description

@IvanaGyro

(Issue written by Claude on behalf of @IvanaGyro.)

Context

PR #760 was merged (merge commit 24e8cc6c, current master HEAD). Its review — #760 (review) (by @pcchen) — raised 9 items that were listed under "Recommended Action Before Merge". I re-checked each against current master: all of them are still present. Filing this so they aren't lost now that the PR is merged. Line numbers below are on master @ 24e8cc6c.

Critical

1. Missing LHS cast in the MKL contract path (fragile correctness)src/BlockUniTensor.cpp:1040 (#ifdef UNI_MKL block), src/BlockFermionicUniTensor.cpp:1614.
The cast guard only casts RHS (if (Rtn->dtype() != common_dtype) …); it never casts this->_blocks when this->dtype() != common_dtype. For real.contract(complex) the LHS stays Double and RHS is already ComplexDouble, so linalg::Matmul(Double_blocks, ComplexDouble_blocks) is called with mismatched types. It currently works only because Matmul promotes internally — undocumented side-effect that breaks if that promotion changes or the gemm_batch path is re-enabled. Cast LHS symmetrically (or document the dependency explicitly).

2. Scalar (all-legs) contraction path ignores common_dtypesrc/BlockUniTensor.cpp:~847, src/BlockFermionicUniTensor.cpp:~1402.
The scalar branch assigns tmp->_block = linalg::Vectordot(...) (and +=) without initializing from common_dtype; the output dtype is whatever Vectordot returns, so the common_dtype fix has no effect on this path.

Important

3. Dead variables allocated on every contraction callsrc/BlockUniTensor.cpp:991-997, src/BlockFermionicUniTensor.cpp:1565-1571.
transs, ms, ns, ks, LMems, RMems, CMems, group_size, alphas are sized to Rtn->_blocks.size() every call but unused (the gemm_batch code is commented out). Delete them with the dead code.

4. Large commented-out blockssrc/BlockUniTensor.cpp:~915-925, ~1092-1116, src/BlockFermionicUniTensor.cpp:~1563-1583, ~1671-1700.
The #ifdef UNI_MKL init branch and the gemm_batch loop are kept as comments. Delete if abandoned, or reduce to a // TODO(#<issue>): ….

5. BlockFermionicUniTensor mixed-dtype contract has zero test coveragetests/BlockFermionicUniTensor_test.cpp.
The same four-line fix was applied to both files, but only BlockUniTensor got a regression test (contract_mixed_dtype_order_independent). tests/BlockFermionicUniTensor_test.cpp has no real⊗complex contract test (it only has VectorContract / SimpleTensorContract). Add a mirrored test.

6. Exception safety: tmpp leaks if astype throwssrc/BlockUniTensor.cpp:1041-1047, src/BlockFermionicUniTensor.cpp:1615-1620.

BlockUniTensor *tmpp = Rtn->clone_meta(true, true);          // raw owning ptr
for (...) tmpp->_blocks[blk] = Rtn->_blocks[blk].astype(common_dtype);  // can throw
tmp_Rtn = tmpp; tmp_rtn_is_casted = true;

If astype throws mid-loop, tmpp leaks. Pre-existing, but this PR added a new throw-capable site. Use a smart pointer / RAII.

Suggestions

7. contract_mixed_dtype_order_independent only exercises Double/ComplexDouble. type_promote(Float, ComplexFloat) is a distinct path — add a parameterized Float/ComplexFloat case.

8. The mixed-dtype fix touched both the matrix-contraction and outer-product (no shared index) branches, but the test only covers the shared-index branch. Add an outer-product mixed-dtype case.

9. Add an explicit dtype() assertion on a same-type contract (e.g. in contract2) to make the same-dtype invariant explicit.

Verification

Checked on master @ 24e8cc6c by inspecting the cited locations; greps confirm the dead variables, RHS-only cast guard, commented gemm_batch, and the absence of a Fermionic mixed-dtype contract test.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions