(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_dtype — src/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 call — src/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 blocks — src/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 coverage — tests/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 throws — src/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.
(Issue written by Claude on behalf of @IvanaGyro.)
Context
PR #760 was merged (merge commit
24e8cc6c, currentmasterHEAD). Its review — #760 (review) (by @pcchen) — raised 9 items that were listed under "Recommended Action Before Merge". I re-checked each against currentmaster: all of them are still present. Filing this so they aren't lost now that the PR is merged. Line numbers below are onmaster @ 24e8cc6c.Critical
1. Missing LHS cast in the MKL contract path (fragile correctness) —
src/BlockUniTensor.cpp:1040(#ifdef UNI_MKLblock),src/BlockFermionicUniTensor.cpp:1614.The cast guard only casts RHS (
if (Rtn->dtype() != common_dtype) …); it never caststhis->_blockswhenthis->dtype() != common_dtype. Forreal.contract(complex)the LHS staysDoubleand RHS is alreadyComplexDouble, solinalg::Matmul(Double_blocks, ComplexDouble_blocks)is called with mismatched types. It currently works only becauseMatmulpromotes internally — undocumented side-effect that breaks if that promotion changes or thegemm_batchpath is re-enabled. Cast LHS symmetrically (or document the dependency explicitly).2. Scalar (all-legs) contraction path ignores
common_dtype—src/BlockUniTensor.cpp:~847,src/BlockFermionicUniTensor.cpp:~1402.The scalar branch assigns
tmp->_block = linalg::Vectordot(...)(and+=) without initializing fromcommon_dtype; the output dtype is whateverVectordotreturns, so thecommon_dtypefix has no effect on this path.Important
3. Dead variables allocated on every contraction call —
src/BlockUniTensor.cpp:991-997,src/BlockFermionicUniTensor.cpp:1565-1571.transs, ms, ns, ks, LMems, RMems, CMems, group_size, alphasare sized toRtn->_blocks.size()every call but unused (thegemm_batchcode is commented out). Delete them with the dead code.4. Large commented-out blocks —
src/BlockUniTensor.cpp:~915-925, ~1092-1116,src/BlockFermionicUniTensor.cpp:~1563-1583, ~1671-1700.The
#ifdef UNI_MKLinit branch and thegemm_batchloop are kept as comments. Delete if abandoned, or reduce to a// TODO(#<issue>): ….5.
BlockFermionicUniTensormixed-dtype contract has zero test coverage —tests/BlockFermionicUniTensor_test.cpp.The same four-line fix was applied to both files, but only
BlockUniTensorgot a regression test (contract_mixed_dtype_order_independent).tests/BlockFermionicUniTensor_test.cpphas no real⊗complex contract test (it only hasVectorContract/SimpleTensorContract). Add a mirrored test.6. Exception safety:
tmppleaks ifastypethrows —src/BlockUniTensor.cpp:1041-1047,src/BlockFermionicUniTensor.cpp:1615-1620.If
astypethrows mid-loop,tmppleaks. Pre-existing, but this PR added a new throw-capable site. Use a smart pointer / RAII.Suggestions
7.
contract_mixed_dtype_order_independentonly exercisesDouble/ComplexDouble.type_promote(Float, ComplexFloat)is a distinct path — add a parameterizedFloat/ComplexFloatcase.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. incontract2) to make the same-dtype invariant explicit.Verification
Checked on
master @ 24e8cc6cby inspecting the cited locations; greps confirm the dead variables, RHS-only cast guard, commentedgemm_batch, and the absence of a Fermionic mixed-dtype contract test.