Fix mixed real/complex BlockUniTensor contract order dependency (#758)

[yingjerkao]

Summary

• fix BlockUniTensor::contract to initialize result tensors with promoted dtype using Type.type_promote(lhs.dtype(), rhs.dtype())
• fix MKL cast path to cast RHS temporary blocks to the promoted dtype instead of lhs dtype
• apply the same dtype-promotion logic to BlockFermionicUniTensor::contract for consistency
• add a regression test for mixed real/complex QN contraction order-independence

Testing

• cmake --build --preset openblas-cpu --target test_main -- -j8
• ./build/openblas-cpu/tests/test_main --gtest_filter=BlockUniTensorTest.contract_mixed_dtype_order_independent
• ./build/openblas-cpu/tests/test_main --gtest_filter=BlockUniTensorTest.contract*
• ./build/openblas-cpu/tests/test_main --gtest_filter=BlockFermionicUniTensorTest.VectorContract:BlockFermionicUniTensorTest.SimpleTensorContract

Fixes #758

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 35.47%. Comparing base (fa015d4) to head (8a92a14).
⚠️ Report is 14 commits behind head on master.

Files with missing lines	Patch %	Lines
src/BlockFermionicUniTensor.cpp	60.00%	3 Missing and 3 partials ⚠️
src/BlockUniTensor.cpp	75.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #760      +/-   ##
==========================================
+ Coverage   35.44%   35.47%   +0.02%     
==========================================
  Files         215      214       -1     
  Lines       33071    33063       -8     
  Branches    13170    13170              
==========================================
+ Hits        11723    11730       +7     
+ Misses      19424    19406      -18     
- Partials     1924     1927       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[IvanaGyro]

Is there any reason to leave the commented code? We can revert it from git history if we need it again in the future. If there is a reason to do this, I suggest leave a comment for the reason.

[manuschneider]

I did not delete these parts because I did not write them, and I am not sure what is happening here. If no one needs this currently, we can certainly delete it.

[pcchen]

PR #760 Review — Fix mixed real/complex BlockUniTensor contract order dependency

Posted by Claude Code on behalf of @pcchen

Critical Issues (2)

1. Missing LHS cast in MKL path — fragile correctness
src/BlockUniTensor.cpp:1044, src/BlockFermionicUniTensor.cpp:1710

The cast guard only casts RHS when Rtn->dtype() != common_dtype. It never casts this->_blocks when this->dtype() != common_dtype. For the case real.contract(complex): RHS is already ComplexDouble so no cast happens, and linalg::Matmul(Double_blocks, ComplexDouble_blocks) is called with mismatched types. This currently produces correct results by accident — Matmul has internal type promotion — but this is an undocumented side-effect. If Matmul's promotion behavior changes, or if the gemm_batch path is re-enabled, this will silently break. The fix should symmetrically cast LHS when this->dtype() != common_dtype.

2. Scalar (all-legs) contraction path ignores common_dtype
src/BlockUniTensor.cpp:~854-859, src/BlockFermionicUniTensor.cpp:~1499-1509

The scalar branch sets tmp->_block directly from linalg::Vectordot(...) without initializing with common_dtype. The output block dtype is entirely determined by Vectordot's return type — the fix has no effect on this path.

---

Important Issues (4)

3. Dead variables allocated on every contraction call
src/BlockUniTensor.cpp:~1000-1007, src/BlockFermionicUniTensor.cpp:~1663-1670

transs, ms, ns, ks, LMems, RMems, CMems, group_size, and alphas are allocated proportional to Rtn->_blocks.size() on every call but are entirely unused because the gemm_batch code is now commented out. These should be deleted along with the dead commented-out code, not left as silent memory waste.

4. Large blocks of commented-out code should be deleted
src/BlockUniTensor.cpp:~919-939, ~1094-1132, src/BlockFermionicUniTensor.cpp:~1563-1583, ~1756-1801

The #ifdef UNI_MKL initialization branch and the gemm_batch loop are preserved as comments. If this code is abandoned, delete it. If it's deferred work, replace with a // TODO(#issue): ... note at most.

5. BlockFermionicUniTensor mixed-dtype contract: zero test coverage
tests/BlockFermionicUniTensor_test.cpp

The identical four-line fix was applied to both files but only BlockUniTensor has a regression test. A mirrored test in BlockFermionicUniTensor_test.cpp is needed.

6. Exception safety: tmpp leaks if astype throws
src/BlockUniTensor.cpp:~1046-1052, src/BlockFermionicUniTensor.cpp:~1709-1715

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 on any iteration, tmpp leaks. Pre-existing issue but worsened by this PR adding a new throw-capable site.

---

Suggestions (3)

7. Float/ComplexFloat not tested — type_promote(Float, ComplexFloat) is a distinct code path; add a second parameterized case.

8. Outer product (no shared index) path not tested — both branches of the contraction received the common_dtype fix but the test only exercises the matrix-contraction branch.

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

---

Strengths

• The core fix (type_promote for output init) is correct and clearly targets the root cause.
• tmp_rtn_is_casted flag is a clean fix to the pre-existing buggy delete condition.
• The Init code deduplication (moving size[i] assignment outside the if/else) is a good cleanup.
• The new test correctly checks both orderings for dtype and value, and EXPECT_NO_THROW is good practice.

---

Recommended Action Before Merge

1. Add symmetric LHS cast in the MKL path (or document the Matmul promotion dependency explicitly)
2. Apply common_dtype to the scalar Vectordot branch
3. Delete dead variables and commented-out gemm_batch code (or track with a TODO issue)
4. Add a BlockFermionicUniTensor mixed-dtype contract test