fix(linalg)!: CPU Add/Sub/Mul/Div/Cpr/Mod typed-storage dispatch + true-division (#941)#1056
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors mixed-dtype arithmetic and true-division on CPU to resolve issue #941. It replaces legacy untyped internal CPU kernels with typed StorageImplementation signatures, ensuring that in-place operations correctly promote the LHS tensor's dtype instead of truncating, and that division follows Python true-division semantics. Floor division bindings have been removed, and extensive tests have been added. The review feedback correctly identifies several style guide violations where small/scalar types (such as cytnx_uint64, bool, and template element types) are passed by const& instead of by value, and where legacy cytnx_uint64 is propagated for ordinary sizes in touched code.
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.
| inline void AddInternalImpl(const boost::intrusive_ptr<StorageImplementation<TO>> &out, | ||
| const boost::intrusive_ptr<StorageImplementation<TL>> &lhs, | ||
| const boost::intrusive_ptr<StorageImplementation<TR>> &rhs, | ||
| const cytnx_uint64 &len, const std::vector<cytnx_uint64> &shape, |
There was a problem hiding this comment.
According to the Cytnx repository style guide, small/scalar types should be passed by value rather than by const&. Please change const cytnx_uint64 &len to cytnx_uint64 len.
| const cytnx_uint64 &len, const std::vector<cytnx_uint64> &shape, | |
| cytnx_uint64 len, const std::vector<cytnx_uint64> &shape, |
References
- Pass small/scalar types by value, not by const&. For built-ins, enums, cytnx_uint64, Scalar, and even complex, by-value is never slower in Cytnx and is often faster. (link)
There was a problem hiding this comment.
Done in a8f2988 — the kernel now takes cytnx_uint64 len by value.
| inline void CprInternalImpl(const boost::intrusive_ptr<StorageImplementation<cytnx_bool>> &out, | ||
| const boost::intrusive_ptr<StorageImplementation<TL>> &lhs, | ||
| const boost::intrusive_ptr<StorageImplementation<TR>> &rhs, | ||
| const cytnx_uint64 &len, const std::vector<cytnx_uint64> &shape, |
There was a problem hiding this comment.
According to the Cytnx repository style guide, small/scalar types should be passed by value rather than by const&. Please change const cytnx_uint64 &len to cytnx_uint64 len.
| const cytnx_uint64 &len, const std::vector<cytnx_uint64> &shape, | |
| cytnx_uint64 len, const std::vector<cytnx_uint64> &shape, |
References
- Pass small/scalar types by value, not by const&. For built-ins, enums, cytnx_uint64, Scalar, and even complex, by-value is never slower in Cytnx and is often faster. (link)
There was a problem hiding this comment.
Done in a8f2988 — the kernel now takes cytnx_uint64 len by value.
| inline void DivInternalImpl(const boost::intrusive_ptr<StorageImplementation<TO>> &out, | ||
| const boost::intrusive_ptr<StorageImplementation<TL>> &lhs, | ||
| const boost::intrusive_ptr<StorageImplementation<TR>> &rhs, | ||
| const cytnx_uint64 &len, const std::vector<cytnx_uint64> &shape, |
There was a problem hiding this comment.
According to the Cytnx repository style guide, small/scalar types should be passed by value rather than by const&. Please change const cytnx_uint64 &len to cytnx_uint64 len.
| const cytnx_uint64 &len, const std::vector<cytnx_uint64> &shape, | |
| cytnx_uint64 len, const std::vector<cytnx_uint64> &shape, |
References
- Pass small/scalar types by value, not by const&. For built-ins, enums, cytnx_uint64, Scalar, and even complex, by-value is never slower in Cytnx and is often faster. (link)
There was a problem hiding this comment.
Done in a8f2988 — the kernel now takes cytnx_uint64 len by value.
| inline void ModInternalImpl(const boost::intrusive_ptr<StorageImplementation<TO>> &out, | ||
| const boost::intrusive_ptr<StorageImplementation<TL>> &lhs, | ||
| const boost::intrusive_ptr<StorageImplementation<TR>> &rhs, | ||
| const cytnx_uint64 &len, const std::vector<cytnx_uint64> &shape, |
There was a problem hiding this comment.
According to the Cytnx repository style guide, small/scalar types should be passed by value rather than by const&. Please change const cytnx_uint64 &len to cytnx_uint64 len.
| const cytnx_uint64 &len, const std::vector<cytnx_uint64> &shape, | |
| cytnx_uint64 len, const std::vector<cytnx_uint64> &shape, |
References
- Pass small/scalar types by value, not by const&. For built-ins, enums, cytnx_uint64, Scalar, and even complex, by-value is never slower in Cytnx and is often faster. (link)
There was a problem hiding this comment.
Done in a8f2988 — the kernel now takes cytnx_uint64 len by value.
| inline void MulInternalImpl(const boost::intrusive_ptr<StorageImplementation<TO>> &out, | ||
| const boost::intrusive_ptr<StorageImplementation<TL>> &lhs, | ||
| const boost::intrusive_ptr<StorageImplementation<TR>> &rhs, | ||
| const cytnx_uint64 &len, const std::vector<cytnx_uint64> &shape, |
There was a problem hiding this comment.
According to the Cytnx repository style guide, small/scalar types should be passed by value rather than by const&. Please change const cytnx_uint64 &len to cytnx_uint64 len.
| const cytnx_uint64 &len, const std::vector<cytnx_uint64> &shape, | |
| cytnx_uint64 len, const std::vector<cytnx_uint64> &shape, |
References
- Pass small/scalar types by value, not by const&. For built-ins, enums, cytnx_uint64, Scalar, and even complex, by-value is never slower in Cytnx and is often faster. (link)
There was a problem hiding this comment.
Done in a8f2988 — the kernel now takes cytnx_uint64 len by value.
| inline void SubInternalImpl(const boost::intrusive_ptr<StorageImplementation<TO>> &out, | ||
| const boost::intrusive_ptr<StorageImplementation<TL>> &lhs, | ||
| const boost::intrusive_ptr<StorageImplementation<TR>> &rhs, | ||
| const cytnx_uint64 &len, const std::vector<cytnx_uint64> &shape, |
There was a problem hiding this comment.
According to the Cytnx repository style guide, small/scalar types should be passed by value rather than by const&. Please change const cytnx_uint64 &len to cytnx_uint64 len.
| const cytnx_uint64 &len, const std::vector<cytnx_uint64> &shape, | |
| cytnx_uint64 len, const std::vector<cytnx_uint64> &shape, |
References
- Pass small/scalar types by value, not by const&. For built-ins, enums, cytnx_uint64, Scalar, and even complex, by-value is never slower in Cytnx and is often faster. (link)
There was a problem hiding this comment.
Done in a8f2988 — the kernel now takes cytnx_uint64 len by value.
| } | ||
| // op_code: 0=Add, 1=Mul, 2=Sub, 3=Div (true division). | ||
| template <char op_code, typename TO, typename TL, typename TR> | ||
| inline TO ApplyInplaceArithmeticOp(const TL &lhs, const TR &rhs) { |
There was a problem hiding this comment.
According to the Cytnx repository style guide, small/scalar types should be passed by value rather than by const&. Since TL and TR represent scalar types of elements, they should be passed by value.
| inline TO ApplyInplaceArithmeticOp(const TL &lhs, const TR &rhs) { | |
| inline TO ApplyInplaceArithmeticOp(TL lhs, TR rhs) { |
References
- Pass small/scalar types by value, not by const&. For built-ins, enums, cytnx_uint64, Scalar, and even complex, by-value is never slower in Cytnx and is often faster. (link)
There was a problem hiding this comment.
Done in 67f0135 — ApplyInplaceArithmeticOp(TL lhs, TR rhs) now passes the element types by value.
| const bool &rhs_is_scalar, | ||
| template <char op_code, typename TO, typename TL, typename TR> | ||
| inline void ApplyInplaceArithmeticKernel(TO *out, const TL *lhs, const TR *rhs, | ||
| const cytnx_uint64 &len, const bool &rhs_is_scalar, |
There was a problem hiding this comment.
According to the Cytnx repository style guide, small/scalar types should be passed by value rather than by const&. Please change const cytnx_uint64 &len and const bool &rhs_is_scalar to be passed by value.
| const cytnx_uint64 &len, const bool &rhs_is_scalar, | |
| cytnx_uint64 len, bool rhs_is_scalar, |
References
- Pass small/scalar types by value, not by const&. For built-ins, enums, cytnx_uint64, Scalar, and even complex, by-value is never slower in Cytnx and is often faster. (link)
There was a problem hiding this comment.
Done in 67f0135 — ApplyInplaceArithmeticKernel now takes cytnx_uint64 len, bool rhs_is_scalar by value.
| const std::vector<cytnx_uint64> &shape, | ||
| const std::vector<cytnx_uint64> &invmapper_L, | ||
| const std::vector<cytnx_uint64> &invmapper_R) { | ||
| const cytnx_uint64 len = Lt._impl->storage()._impl->size(); |
There was a problem hiding this comment.
According to the Cytnx repository style guide, legacy cytnx_XXXX typedefs should not be propagated as ordinary sizes or loop counters in new or touched code. Please use standard C++ types like uint64_t or std::size_t instead of cytnx_uint64.
| const cytnx_uint64 len = Lt._impl->storage()._impl->size(); | |
| const uint64_t len = Lt._impl->storage()._impl->size(); |
References
- For new or touched code, avoid propagating legacy cytnx_XXXX typedefs as ordinary loop counters, sizes, flags, or local arithmetic types. Prefer standard C++ types unless the value is specifically a dtype-backed tensor/storage scalar. (link)
There was a problem hiding this comment.
Done in 67f0135 — the len local is now uint64_t (cytnx_uint64 is a typedef uint64_t, so no functional change; kept cytnx_uint64 on the by-value kernel param per your other suggestion).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23b6c1c561
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def test_unitensor_floordiv_unbound(): | ||
| ut = cytnx.UniTensor(cytnx.ones([2, 2])) | ||
| with pytest.raises(TypeError): | ||
| ut // 2 |
There was a problem hiding this comment.
This assertion will fail because pybind/unitensor_py.cpp still explicitly binds scalar __floordiv__, __rfloordiv__, and __ifloordiv__ for UniTensor and documents that scalar floordiv stays available, so ut // 2 returns linalg::Div(self, 2) rather than raising TypeError. Either remove/update those UniTensor bindings too, or keep this regression test scoped to Tensor only.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is now resolved on the current head. The test was retargeted after the reviewed commit (23b6c1c): test_unitensor_floordiv_scalar_kept_tensor_raises keeps scalar ut // 2 (routes to Div, mirroring scalar /) and only raises on ut // ut, consistent with the UniTensor __floordiv__ bindings and #1049. Plain-Tensor // stays fully unbound (test_tensor_floordiv_unbound). Both pass.
23b6c1c to
bd87d02
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1056 +/- ##
===========================================
+ Coverage 60.49% 72.11% +11.61%
===========================================
Files 229 225 -4
Lines 31873 28207 -3666
Branches 71 71
===========================================
+ Hits 19282 20341 +1059
+ Misses 12570 7845 -4725
Partials 21 21
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd87d02b01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // added in #1026. | ||
| if (detail::init_broadcast_binary_output( | ||
| out, Lt, Rt, | ||
| Type_class::make_floating_point_dtype(Type.type_promote(Lt.dtype(), Rt.dtype())))) { |
There was a problem hiding this comment.
When both operands are CUDA integral/bool tensors, this allocates a floating output for true division, but the CUDA path below still calls cuDiv_dispatch; I checked src/backend/linalg_internal_gpu/cuDiv_dispatch.cu:209 and it still requires Type.type_promote(Lin->dtype(), Rin->dtype()) and instantiates its kernel output from type_promote_gpu_t. As a result, gpu_int / gpu_int (and the scalar integer overloads changed the same way) will throw an output dtype mismatch instead of returning the new Double result, so the CUDA dispatch needs the same true-division dtype rule before this allocation is used on GPU.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 3afbbce. The output storage was seeded floating for every device, but the GPU path still runs the legacy cuDiv_dispatch, which asserts out->dtype() == Type.type_promote(lhs, rhs) and instantiates from type_promote_gpu_t, so gpu_int / gpu_int (and the scalar integer overloads) would throw. Introduced a device-aware div_output_dtype helper: CPU keeps the floating true-division result, GPU keeps the promoted (possibly integer) dtype — matching cuDiv_dispatch and leaving GPU behavior identical to master (CPU-only scope, #1013 tracks GPU). Verified the CUDA library compiles clean. Note this makes integer division diverge by device (CPU int/int→Double, GPU int/int→int) — flagged for the maintainer per the type-promotion guardrail.
|
This is a substantial correctness improvement and a good start: typed storage removes the unsafe erased dispatch, and deleting more than 13,000 lines of legacy tables is valuable. However, I do not think it goes far enough architecturally. The six CPU headers still contain essentially the same traversal kernel:
Each independently implements scalar-left, scalar-right, contiguous, and permuted traversal. Only the element operation and result-type policy differ. The non-contiguous path is especially concerning: on every element it constructs the result of The in-place implementation is closer to the right structure because it shares The common primitive should look roughly like: template <class TO, class TL, class TR, class Op>
void binary_transform(TO* out, const TL* lhs, const TR* rhs,
const BinaryLayout& layout, Op op);
The layout should describe the output, LHS, and RHS mappings or strides. Then out-of-place and in-place operations can use the same kernel: an in-place operation simply uses the LHS layout as the output layout. Scalar broadcasting can be represented by a zero stride or another explicit layout state, with any useful scalar/contiguous fast paths implemented once inside the common traversal. The public Most importantly, #1013 should not port these six nearly identical kernels to CUDA one by one. CPU and CUDA need backend-specific traversal implementations, but they should consume the same operation functors and dtype policies. Otherwise the current duplication will immediately be reproduced on the GPU side. I would regard this PR as useful and mergeable groundwork, but not as the completed arithmetic refactor. Ideally the common binary transform should be introduced here; at minimum it should be the first step of #1013 rather than copying the per-operation structure. Written by OpenAI Codex and posted at @ianmccul's request. |
|
I would actually consider implementing scalar broadcast with another kernel function, but it only needs to be done once, eg Tensor op Scalar, and the Scalar Op Tensor case can re-use that kernel but with the Op reversed, eg SubReversed, DivReversed, etc. The reason being that decoding the layout only needs to be done for the Tensor arguments. |
…ue-division (#941) Reimplements the deferred half of #1015 on current master, which had since refactored the same six arithmetic files for rank-zero support (#1026), zero-extent (#1043), and type_promote (#984). - Convert CPU out-of-place AND in-place Add/Sub/Mul/Div/Cpr/Mod to typed std::visit over as_storage_variant() with storage_cast<TO> and typed *InternalImpl<TO,TL,TR> kernels; the dead legacy dispatch tables are removed. - True division (#941): `/` and `/=` yield a floating result (int/int -> Double) via make_floating_point_dtype; the rank-zero / broadcast output still routes through #1026's init_broadcast_binary_output (seeded with the floating dtype). - Mixed-dtype in-place TENSOR ops promote the LHS via storage_as_type_or_replace (no more truncation into the narrower LHS storage -- #941's core bug). - Weak-scalar (#980, per #1015's binding ruling): `tensor op= python-scalar` (a rank-0 RHS via scalar_as_rank0_tensor) PRESERVES the LHS dtype for Add/Sub/Mul and follows true-division for Div (Int64 /= 2.0 -> Double, Float /= 3.0 -> Float), keyed on Rt.rank()==0 in DispatchInplaceArithmeticCPU; complex-into-real still throws. - floordiv conforms to #1049 (scalar `ut // 2.0` kept, `ut1 // ut2` raises); #1015's original unbind-all is dropped. - normalize_() now promotes an integer block to Double under true division; its stale "stays Int32" comment is updated. Mod keeps an eager is_complex guard so a zero-extent complex Mod still throws (ZeroExtentLinalgTest). CPU only; GPU stays on the legacy dtype tables behind the unchanged public API (#1013 tracks the GPU conversion). Testing: full openblas-cpu (non-ASan) test_main 1784 passed / 11 skipped / 0 failed -- MixedDtypeArithmetic regressions, weak-scalar Tensor.ScalarInplace*, ZeroExtentLinalgTest, and DenseUniTensorTest true-division/normalize all green; clang-format v14 clean. Reimplements ff30d00, deferred from #1015 (#941). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bd87d02 to
8fa9fac
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fa9fac3e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // it PRESERVES the LHS dtype (the scalar is cast into TL) rather than | ||
| // promoting. A rank>=1 RHS is a genuine tensor and promotes (#941). | ||
| // Complex-into-real is still rejected by ApplyInplaceArithmeticOp. | ||
| const bool weak_scalar_rhs = (Rt.rank() == 0); |
There was a problem hiding this comment.
When the RHS is an actual rank-0 Tensor (for example cytnx.zeros([], dtype=Type.Double)) rather than a Python scalar, this test still sends it down the weak-scalar path and forces TO_weak to ignore the RHS dtype. That makes cases like an Int32 tensor +=/-= a rank-0 Double tensor truncate and keep Int32, while the equivalent out-of-place op and a shape-[1] tensor RHS promote to Double; rank-0 tensors are first-class tensor values in this codebase, so the scalar-wrapper special case needs a way to distinguish wrapper calls from user-provided rank-0 tensors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 67f0135. Rather than inferring weak-scalar from Rt.rank()==0 (which can't distinguish a python-scalar wrapper from a user's cytnx.zeros([])), the scalar in-place operators now pass an explicit rhs_is_weak_scalar flag through iAdd/iSub/iMul/iDiv into the dispatcher. A python scalar preserves the lhs dtype (#980); any genuine tensor RHS — rank-0 included — promotes (#941), matching the out-of-place op and a shape-[1] RHS. Added a pytest pinning the rank-0-tensor promotion (and the contrasting python-scalar preservation), verified to fail on the pre-fix rank-based detection.
Address PR #1056 review (Gemini): the typed CPU Add/Sub/Mul/Div/Cpr/Mod InternalImpl kernels took the element count as `const cytnx_uint64 &len`. Per the Cytnx style guide, small scalar types pass by value (a value rides in registers; const& forces a stack spill + indirection). No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #1056 review (Codex P1): true division seeded the Div output storage with make_floating_point_dtype(type_promote(...)) for every device. The GPU path, however, still runs the legacy cuDiv_dispatch, which validates `out->dtype() == Type.type_promote(lhs, rhs)` and instantiates its kernel output from type_promote_gpu_t. A floating output therefore made `gpu_int / gpu_int` (and the scalar integer overloads) throw an output-dtype-mismatch instead of returning a result. Add a device-aware div_output_dtype helper: CPU keeps the floating-point true-division result, while the GPU keeps the promoted (possibly integer) dtype, matching cuDiv_dispatch and restoring master's GPU behavior. This preserves the PR's CPU-only scope (#1013 tracks the GPU true-division conversion). NOTE (numerical behavior): integer division now diverges by device -- CPU int/int -> Double (true division), GPU int/int -> int (legacy truncation, unchanged from master). Flagging per the type-promotion guardrail. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ic (#941) Address PR #1056 review (Codex P2): DispatchInplaceArithmeticCPU inferred numpy weak-scalar semantics from `Rt.rank() == 0`, but a user-provided rank-0 tensor (e.g. cytnx.zeros([])) is also rank 0. That made `int_tensor += rank0_double_tensor` truncate into the narrower lhs, while the out-of-place op and a shape-[1] tensor RHS promote -- an inconsistency, since rank-0 tensors are first-class tensor values in Cytnx. Thread an explicit `rhs_is_weak_scalar` flag from the scalar in-place operators (which wrap the scalar via scalar_as_rank0_tensor) through iAdd/iSub/iMul/iDiv into the dispatcher, instead of guessing from rank. A python-scalar RHS preserves the lhs dtype (numpy weak scalar, #980); any genuine tensor RHS -- rank-0 included -- promotes (#941). The public iAdd/iSub/iMul/iDiv gain a defaulted `rhs_is_weak_scalar = false` param, so existing callers are unaffected. Adds a pytest pinning both the rank-0-tensor promotion (verified to fail on the pre-fix rank-based detection) and the contrasting python-scalar preservation. Also folds in the PR #1056 pass-by-value style fixes in this file (ApplyInplaceArithmeticOp element params, kernel len/rhs_is_scalar, uint64_t len local). NOTE (numerical behavior): a new observable mixed-dtype rule -- an in-place op against a rank-0 tensor operand now promotes on CPU. Flagging per the type-promotion guardrail. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67f013587b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.cast<cytnx::Tensor &>().Div_(rhs); | ||
| return self; | ||
| }) | ||
| // __floordiv__/__rfloordiv__/__ifloordiv__ are deliberately UNBOUND |
There was a problem hiding this comment.
When Tensor floor-division is deliberately unbound here, the committed PEP 561 stub still advertises Tensor.__floordiv__, __rfloordiv__, and __ifloordiv__ overloads in cytnx/cytnx/__init__.pyi (for example the __floordiv__ block starts at line 1423). Users running mypy/Pylance will be told t // 2 is valid even though this change makes it raise TypeError at runtime, so the stubs need to be regenerated or edited with the binding removal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7ea64fa. Removed the stale Tensor floordiv overloads from cytnx/cytnx/__init__.pyi — __floordiv__, __rfloordiv__, __ifloordiv__, plus the keep-set-generated c__ifloordiv__ — each verified absent from the built module (this PR cut tensor_py.cpp's floordiv bindings from 41 refs to 0). t // 2 will now be flagged by mypy/Pylance, matching the runtime TypeError.
I did this as a surgical edit rather than a full tools/generate_stubs.py regeneration: the committed stubs are generated with the lowest supported interpreter (3.10), and this environment only has 3.12 + a newer numpy, so a wholesale regen emits ~2600 lines of unrelated tooling/version churn (dropped typing_extensions import, Bond method drift, getDegeneracy signature reshuffles, etc.) — out of scope for this PR and unreviewable. A maintainer on the pinned 3.10 toolchain can do the clean full regen separately.
Scope note: UniTensor scalar floordiv is intentionally kept (#1049), so its stub entries stay. This PR doesn't touch UniTensor floordiv bindings; the UniTensor __floordiv__(UniTensor) overload is separately stale (ut // ut raises), but that predates this PR (#1049 on master) so I left it alone.
There was a problem hiding this comment.
CI is green on the head commit (7ea64faf) with this stub fix in: 17 checks passing, 0 failing — BuildAndTest across ubuntu-latest / macos-latest / macos-15-intel, all four BuildWheel jobs (ubuntu-24.04, ubuntu-24.04-arm, macos-14, macos-15-intel), Formatting Check (clang-format v14), DownstreamFindPackage, and codecov (patch/project/cpp/python). The only non-passing checks are PublishNightlyAnaconda and ReleasePyPI, both skipped as expected on a PR. Thanks for the catch on the stale stubs.
) Address PR #1056 review (Codex): this PR unbinds Tensor `__floordiv__` / `__rfloordiv__` / `__ifloordiv__` (and the keep-set-generated `c__ifloordiv__`) in pybind/tensor_py.cpp, but the shipped PEP 561 stub cytnx/cytnx/__init__.pyi still advertised those overloads. mypy/Pylance would therefore accept `t // 2` even though it now raises TypeError at runtime. Remove exactly the Tensor floordiv-family entries from the stub, each verified absent from the built module (`__floordiv__`, `__rfloordiv__`, `__ifloordiv__`, `c__ifloordiv__`). This is a scoped surgical edit rather than a full `tools/generate_stubs.py` regeneration: this environment only has Python 3.12 (the committed stubs are generated with the lowest supported 3.10) plus a different numpy, so a wholesale regen emits ~2600 lines of unrelated tooling/version churn. UniTensor scalar floordiv is intentionally kept (#1049) and left untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ea64fafcc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| detail::DispatchInplaceArithmeticCPU<3>(Lt, R, rhs_is_weak_scalar, empty_mapper, | ||
| empty_mapper, empty_mapper); |
There was a problem hiding this comment.
For zero-extent CPU tensors this new true-division dispatch is never reached because iDiv returns earlier when Lt.storage().size() == 0, so the dtype replacement in DispatchInplaceArithmeticCPU is skipped. For example, an empty Int64 tensor divided in place by an empty/rank-0 Int64 tensor or scalar stays Int64, while the equivalent out-of-place / now returns Double and non-empty /= promotes; the same early-return pattern also skips mixed-dtype tensor promotion in iAdd/iSub/iMul. Move the dtype selection/storage replacement before the empty fast return, or let the dispatcher handle len == 0.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2adb13b. The Lt.storage().size() == 0 early return in iAdd/iSub/iMul/iDiv now only short-circuits the legacy GPU path; on CPU the flow continues into DispatchInplaceArithmeticCPU, which runs storage_as_type_or_replace<TO> (the dtype replacement) with a no-op kernel at length 0 — the same 0-size allocation the out-of-place path already performs. So empty_int64 /= empty_int64 now yields Double, empty_int16 += empty_double promotes, etc., matching both the out-of-place op and the non-empty in-place path. GPU keeps the LHS dtype in place regardless (#1013), so I left its early return intact.
Reproduced the bug first (empty /= stayed Int64 vs out-of-place Double), then added a pytest pinning zero-extent promotion for /,+,-,* and weak-scalar preservation.
TensorTest.ZeroExtentArithmeticAndSlicing asserted empty(Float) += scalar(rank-0 Double tensor) stays Float — but that expectation was the pre-rank-0-ruling behavior, previously masked by this same early return. scalar there is a genuine rank-0 Tensor (C++ has no weak-scalar concept for Tensor += Tensor), so it promotes per 67f0135, consistent with the out-of-place empty + scalar → Double assertion in the very same test. Updated it to Double and flagged it in the commit as a numerical-behavior change for maintainer sign-off.
Full test_main (1787) and pytest pytests/ (248 passed, 1 skipped) green locally.
Address PR #1056 review (Codex): the `Lt.storage().size() == 0` early return in iAdd/iSub/iMul/iDiv fired before DispatchInplaceArithmeticCPU, so a zero-extent in-place op skipped the dtype promotion this PR added. An empty `Int64 /= Int64` stayed Int64 while the out-of-place `/` and non-empty `/=` now yield Double; the same early return masked mixed-dtype promotion for `+=`/`-=`/`*=`. Short-circuit only the legacy GPU path on empty and let the CPU dispatcher run: storage_as_type_or_replace performs the dtype replacement with a no-op kernel at length 0 (the same 0-size allocation the out-of-place path already does). GPU keeps the LHS dtype in place regardless (#1013). Also updates TensorTest.ZeroExtentArithmeticAndSlicing: `empty(Float) += scalar(rank-0 Double tensor)` now promotes to Double, matching the out-of-place `empty + scalar` assertion in the same test and the non-empty path. `scalar` is a genuine rank-0 tensor (C++ has no weak-scalar concept for `Tensor += Tensor`), so it promotes per the rank-0 ruling already in this PR (67f0135); the stale Float expectation was previously masked by the empty early return. Adds a pytest pinning zero-extent promotion for /,+,-,* plus weak-scalar preservation. NOTE (numerical behavior): zero-extent in-place arithmetic now promotes dtype on CPU (was: preserved). Flagging per the type-promotion guardrail. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ianmccul
left a comment
ianmccul
left a comment
There was a problem hiding this comment.
ok, but there is still a lot of duplication. Suggest widen #1067 to merge GPU and CPU paths, and route through a small number of common kernels that are parameterized on the operation, AddOp, SubOp, MulOp, TrueDivOp, EqualOp, ModOp etc
2 participants
Problem
Follow-up to #1015, which landed the typed-storage foundation (
StorageVariant,storage_cast,as_storage_variant) but deferred its CPU linalg conversion + true-division commit (ff30d0090) because it collided with master's #1026 rank-zero refactor of the six arithmetic files. This PR reimplements that deferred work on top of currentmaster.What this does (#941)
Converts the CPU
Add/Sub/Mul/Div/Cpr/Mod(out-of-place and in-place) fromStorage_base*+reinterpret_castdispatch to typedstd::visitoveras_storage_variant()withstorage_cast<TO>and typed*InternalImpl<TO, TL, TR>kernels. Two behavioral changes fall out (both are BREAKING and were ruled on in #941 / #1015):/and/=yield a floating-point result (int/int → Double) viamake_floating_point_dtype(type_promote(...)). Rank-zero / broadcast output allocation still goes through master'sinit_broadcast_binary_output(fix(Tensor & UniTensor): support rank-zero tensors and zero extents #1026), just seeded with the floating dtype.Int16 -= Double_tensor → Double(no more silent truncation into the narrower LHS storage, Its time: Typed Storage Dispatch #941's core bug), viastorage_as_type_or_replace<TO>inDispatchInplaceArithmeticCPU.Weak-scalar preservation (#980, per #1015's binding ruling)
tensor op= python-scalarmust preserve the LHS dtype (numpy weak-scalar), not promote. Since the scalar operators wrap the scalar as a rank-0 tensor (scalar_as_rank0_tensor),DispatchInplaceArithmeticCPUkeys onRt.rank() == 0to keepTLfor a scalar RHS while a rank≥1 tensor RHS promotes.Float += 1.0stays Float;Int64 += 2.7stays Int64 (=3); real+= complexstill throws. This is a new observable numerical rule — flagging for review.Floordiv
//conforms to #1049 (kept on master): scalarut // 2.0stays,ut1 // ut2raisesTypeError. #1015's original "unbind//entirely" is dropped.Flag: floordiv Tensor/UniTensor asymmetry
With true division,
//aliasingDivwould silently do true division (5 // 2 -> 2.5), which is worse than the old truncation. So per #941 plainTensor //stays fully unbound (raisesTypeError), while per #1049UniTensorscalar//is kept (routes toDiv, mirroring scalar/) andut // utraises. This asymmetry reflects the decisions on record (#941 for Tensor is unsettled; #1049 settled UniTensor) rather than a new ruling — flagging in case you want to reconcile the two.Scope
CPU only. GPU stays on the legacy dtype tables behind the unchanged public API (#1013 tracks the GPU conversion).
Testing
MixedDtypeArithmeticregressions (in-place promote for +/-/*,Div/iDivtrue-division across Int16/Int32/Int64/Uint16,Cprbool,Modpromote) + master'sTensor.ScalarInplace*weak-scalar tests +DenseUniTensorTest.normalize_int_type(true-division) all pass. Fullopenblas-cpu(non-ASan)test_maingreen; clang-format v14 clean.Closes the deferred half of #1015 (#941).
🤖 Generated with Claude Code