fix(Tensor): scalar in-place arithmetic mutates storage in place

[yingjerkao]

Problem

Part of #906. t += 1.0 (and -=, *=, /= with any scalar or cytnx::Scalar) replaced the tensor's Storage with a freshly computed one:

this->_impl->storage() = cytnx::linalg::Add(*this, rc)._impl->storage();

Any view sharing that storage (from permute(), contiguous reshape(), from_storage()) silently detached, while t += tensor mutated in place — the two paths disagreed on aliasing semantics.

Fix

Scalar RHS now routes through the same iAdd/iSub/iMul/iDiv kernels via a shape-{1} wrapper tensor (the kernels already broadcast a length-1 RHS and mutate LHS storage in place). All 48 scalar specializations rewritten; the Tensor/Tproxy/Sproxy specializations are untouched.

Deliberate behavior changes

• Views sharing storage stay attached (the bug fix).
• LHS dtype is preserved: Float tensor += 1.0 stays Float (previously promoted to Double); integer LHS truncates fractional scalars — both matching the existing tensor-RHS in-place behavior and numpy's scalar rules.
• real_tensor ⊙= complex_scalar now throws instead of silently promoting the tensor to complex.

The GPU iAdd path can still rebind on promotion; that remains tracked under #906.

Testing

Seven new gtests: storage-sharing preserved for all four ops (including through a real non-contiguous permute() view), dtype preservation, integer truncation pinned, real ⊙= complex throws for all four ops, and cytnx::Scalar RHS. Red-state verified against the old code (a shared view read 0 after += — update lost). Full suite passes at baseline.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request optimizes scalar in-place arithmetic operations on Tensors by wrapping scalars into rank-1 Tensors to reuse existing in-place kernels, resolving storage sharing issues. It also corrects operator names in mismatch error messages and adds comprehensive unit tests. The review feedback highlights a potential performance bottleneck in the helper function _scalar_as_rank1_tensor, where creating a temporary CPU tensor and transferring it to a GPU device for every scalar operation can introduce significant overhead.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

Creating a temporary Tensor on CPU and then calling .to(device) on it introduces a significant performance bottleneck, especially when device is a GPU. This involves allocating GPU memory (cudaMalloc) and performing a host-to-device copy for every single in-place scalar operation (e.g., a += 1.0;), which is extremely slow and defeats the purpose of in-place operations.

Consider optimizing this by checking if device is GPU and reusing a pre-allocated or cached scalar tensor, or by bypassing the tensor wrapper entirely for CPU/GPU scalar operations where possible.

[codecov]

Codecov Report

❌ Patch coverage is 11.47541% with 54 lines in your changes missing coverage. Please review.
✅ Project coverage is 31.84%. Comparing base (82a9904) to head (dfe688b).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/Tensor.cpp	6.89%	32 Missing and 22 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #980      +/-   ##
==========================================
- Coverage   31.85%   31.84%   -0.01%     
==========================================
  Files         230      230              
  Lines       33107    33117      +10     
  Branches    13840    13846       +6     
==========================================
  Hits        10547    10547              
- Misses      15532    15549      +17     
+ Partials     7028     7021       -7     

Flag	Coverage Δ
cpp	31.48% <11.47%> (-0.01%)	⬇️
python	61.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
C++ backend	32.57% <11.47%> (-0.02%)	⬇️
Python bindings	24.03% <ø> (ø)
Python package	61.84% <ø> (ø)

Files with missing lines	Coverage Δ
src/linalg/iArithmetic_visit.hpp	83.33% <100.00%> (+9.52%)	⬆️
src/Tensor.cpp	21.98% <6.89%> (+2.09%)	⬆️

... and 7 files with indirect coverage changes

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82a9904...dfe688b. Read the comment docs.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[IvanaGyro]

This can be moved to linalg to benefit the linalg functions. If the naming style in the current file is messy, the name of the new function should follow the coding style and avoid prefixing with _.

[ianmccul]

Perhaps a good opportunity make scalar as rank 0 tensors? It would make operations such as trace much more consistent.

[IvanaGyro]

That will make Scalar heavier than a simple std::variant and we may have to triage scalar in all operations consuming tensors

[ianmccul]

No, I meant rank 0 tensor should exist and be "scalar + shared storage + device"; see comment #1016 (comment)

• Scalar is similar to to Python builtin number
• Rank-0 tensor is similar to numpy rank 0 ndarray

[pcchen]

Proposal: defer this to the outcome of #1016 rather than changing it here. Where the scalar-wrapper helper should live (and what it's named) is entangled with the "rank-0 tensor vs. shape-{1} convention" design question now tracked there — #1016's option B explicitly includes exactly the relocation you suggested (a shared linalg-level scalar_tensor() helper as the single implementation point, no _ prefix), while option A would make this helper obsolete altogether. Two stacked PRs also touch this same helper (#992 makes it CPU-resident to fix a GPU UB; #1015's typed-storage direction may reshape it again), so moving it now would just multiply churn.

If that's acceptable, could you re-review/dismiss the requested-changes gate so #980 + #992 (the CUDA use-after-free / UB fix stacked on this) can land? The rename+relocate then happens once, in its final form, per the #1016 decision.

Posted by Claude Code on behalf of @pcchen

[pcchen]

Good concrete argument — captured in #1016 (the design-question issue for rank-0 vs. the shape-{1} convention), where it weighs for option B/C (keep Scalar a lightweight variant, as #1011 builds) against option A (true rank-0 tensors). Let's keep the design debate there so it accumulates in one place; no change on this PR.

Posted by Claude Code on behalf of @pcchen

[pcchen]

Code Review

Fixes the real #906 bug: t += 1.0 did storage() = Add(*this, rc).storage() — computing out-of-place at a promoted dtype and swapping the Storage handle, silently detaching every view sharing that storage — while t += tensor mutated in place. All 48 scalar specializations (+4 Scalar overloads) now route through the in-place iAdd/iSub/iMul/iDiv kernels via a shape-{1} wrapper. LGTM once rebased.

Correctness — verified

• The iOp + shape-{1} route is sound: iDiv et al. explicitly special-case a length-1 RHS (iDiv.cpp:17 skips the shape check for {1}) and dispatch in-place on LHS storage — contiguous and non-contiguous paths — with an aliasing guard (irrelevant here; the wrapper is always fresh). Same established path DenseUniTensor::normalize_ uses. ✅
• _scalar_as_rank1_tensor is correct for CPU + GPU (build on CPU, set element, .to(device)); Tensor::from_storage used by the tests exists (Tensor.hpp:558). ✅
• Perf is a side win: the old path allocated a full N-element result per scalar op; the new one allocates one element.
• Tests are strong: storage-sharing pinned via from_storage (with a sharp comment on why copy-ctor aliasing couldn't observe the bug) and through a real non-contiguous permute() view; dtype preservation; integer truncation; real⊙=complex throws ×4; Scalar RHS; corrected error text. Red-state verified; CI on the pre-conflict commit: 15 pass, only advisory codecov red. ✅

Findings

1. 🟠 CONFLICTING — trivial rebase. Checked the merge-base: src/Tensor.cpp and iArithmetic_visit.hpp have no master-side changes; the only collision is the trailing test-append in tests/Tensor_test.cpp (vs. fix(Tensor): harden reshape validation (multiple -1, 0-dim with -1, strong exception guarantee) #976's reshape tests). I'll resolve it (keep both), as with fix(Tensor): harden reshape validation (multiple -1, 0-dim with -1, strong exception guarantee) #976/fix: drop C complex.h from pybind TUs; rename macro-colliding template param #981.
2. 🟡 Changelog entry needed for the deliberate behavior changes: dtype no longer promotes (Float += 1.0 stays Float; Int64 += 2.7 truncates) and real ⊙= complex throws instead of promoting. numpy-consistent and tensor-RHS-consistent, but downstream code relying on silent promotion will change results.
3. 🟢 Positive cross-PR interactions: making operator/=<double>/<Scalar> dtype-preserving fixes the /=-half of the Float→Double Lanczos regression flagged on feat(api)!: norm()->Scalar, underscore convention for mutators, unified permute/reshape syntax #1000 (the out-of-place clone()/norm() sites there still promote), and gives Block/BlockFermionic normalize_ (block /= Scalar) the dtype-preservation guarantee that previously only Dense had.

On the three inline comments

• @IvanaGyro (move helper to linalg; avoid the _ prefix): both fair, non-blocking. The helper is genuinely reusable (Ger's implicit alpha, the normalize_ divisors); relocating it to linalg forces the rename anyway. Fine here or as a fast-follow.
• @ianmccul (rank-0 scalar tensors): good design question, bigger than this PR — spun off as Design question: should scalars be rank-0 (shape-{}) tensors? #1016 with the options laid out (true rank-0 / document the {1} convention / fold into Replace Scalar raw-pointer polymorphism with value semantics #935). This PR's {1} wrapper is forward-compatible either way — it's the single switch site if rank-0 lands.
• @gemini-code-assist (GPU H2D-per-op bottleneck): valid observation, wrong severity — the old path allocated a full N-element GPU tensor + ran an out-of-place kernel per scalar op; the new path allocates one element + one H2D copy, strictly cheaper for any nontrivial tensor. The per-call GPU overhead is already tracked in GPU: scalar in-place ops throw on non-contiguous tensors for *= and /=, and do a per-call H2D copy #988, and the real fix (dedicated scalar-argument kernels) is Scalar in-place Tensor arithmetic detaches storage-sharing views #906 follow-up territory. One caution on the caching suggestion: a pre-allocated/cached scalar tensor would be mutable shared state — a data race now that feat(pybind): release the GIL around expensive linalg/contract calls #987 releases the GIL around these calls. Any caching must be thread-local or immutable-per-value; better to not cache and go straight to scalar kernels.

Posted by Claude Code on behalf of @pcchen

[gemini-code-assist]

Warning

Gemini encountered an error creating the issue-comment-reply. You can try again by commenting /gemini issue-comment-reply.

[pcchen]

Approving. The fix is verified: all 48 scalar specializations (+4 Scalar overloads) now route through the in-place iAdd/iSub/iMul/iDiv kernels via a shape-{1} wrapper — the kernels explicitly special-case a length-1 RHS and mutate LHS storage in place, so storage-sharing views stay attached (the #906 bug), and the old full-N-element allocation per scalar op is gone. Dtype preservation, integer truncation, and the real ⊙= complex rejection are all pinned by red-state-verified tests, including through a genuine non-contiguous permute() view. CI is functionally green on the rebased head (only advisory codecov red).

Two coordination notes, both agreed in the threads:

1. Land #992 immediately after this — on CUDA, this PR's .to(device) wrapper is read host-side by the GPU kernels (UB); #992 (stacked on this branch, hardware-validated) fixes that plus the non-contiguous GPU scalar regression. The pair should merge back-to-back.
2. The helper relocation/rename requested in review is deferred to #1016 (the rank-0-vs-{1}-convention design issue) rather than churned here — it's exactly option B's "single implementation point" helper there, and #992/#1015 touch the same code. The rename+move happens once, in its final form, per that decision.

Behavior changes (dtype no longer promotes; real ⊙= complex throws) are deliberate, numpy-consistent, and need a changelog entry alongside the other in-place-semantics rulings.

Posted by Claude Code on behalf of @pcchen

Dismissing per the deferral proposed in the review thread: the helper relocation/rename is folded into the #1016 design decision (it is option B's single-implementation-point helper; #992 and #1015 touch the same code), so it will be done once, in its final form, per that outcome rather than churned here. Unblocks the #980→#992 pair (#992 fixes the CUDA host-deref UB stacked on this branch). — dismissed by Claude Code on behalf of @pcchen