fix(pybind): keep-set ordering for Scalar constructors, add __bool__#1014
Closed
yingjerkao wants to merge 2 commits into
Closed
fix(pybind): keep-set ordering for Scalar constructors, add __bool__#1014yingjerkao wants to merge 2 commits into
yingjerkao wants to merge 2 commits into
Conversation
…847, fixes #935) Replace Scalar's Scalar_base* PIMPL (11 derived classes, 108 virtuals, ~2900 LOC, one heap allocation per Scalar) with a std::variant held by value, per #847's proposed layout: std::variant<std::monostate, cytnx_complex128, cytnx_complex64, cytnx_double, cytnx_float, cytnx_int64, cytnx_uint64, cytnx_int32, cytnx_uint32, cytnx_int16, cytnx_uint16, cytnx_bool> std::monostate represents the Void/uninitialized state (dtype() returns Type.Void). ScalarVariant is index-aligned with Type_list -- pinned by static_asserts in the header -- so dtype() is simply the active alternative's index. All public signatures are preserved: the 11 typed ctors, Scalar(T, dtype), explicit conversion operators, astype/conj/abs/sqrt (+ in-place forms), real()/imag(), dtype(), maxval/minval(dtype), print/ostream, all arithmetic and comparison operators, and Scalar::Sproxy. sizeof(Scalar) goes from 8 B (pointer) + a heap object to 24 B fully inline with true value semantics (copy/move/assign are `= default`, trivially correct). #935 fixes (bug catalog credit: @ianmccul): - self-assignment UB in operator=: eliminated by `= default`. - double-allocation copy ctor: eliminated by `= default`. - broken in-place promotion (Int64 /= Double silently doing integer division, RHS converted to destination dtype): in-place ops now throw instead of silently truncating (Ruling 1 below). - mixed-precision in-place float loss (Float += Double): now throws. - minval(Type.Double/Float) returned numeric_limits::min() (smallest positive normal); now returns ::lowest() (most negative finite). - abs()/iabs() dtype inconsistency on complex: abs() always returns the real dtype magnitude (Double/Float). iabs() keeps the complex dtype (no in-place op changes dtype) and stores the magnitude with zero imaginary part; its value now provably matches abs()'s. Documented in the header and pinned by tests. - hand-rolled enum-order promotion everywhere: every promotion decision (binary ops, comparisons, in-place guard) now routes through the single constexpr Type.type_promote() via std::visit; this also makes ComplexFloat + Double correctly yield ComplexDouble. Ruling 1 (maintainer decision 2026-07-07; BREAKING, also visible from Python where it raises RuntimeError): the in-place operators += -= *= /= throw cytnx::error when the RHS dtype does not convert into the LHS dtype, i.e. when Type.type_promote(lhs, rhs) != lhs.dtype(). Throwing classes: unsigned-LHS mixed with signed ints, int-LHS with float RHS, real-LHS with complex RHS, Bool-LHS with non-Bool. Conversions that are value-preserving per the sanctioned type_promote table (see include/Type.hpp type_promote; maintainer sign-off 2026-07-07) remain valid: same dtype; Int64 += Int32; Double += Float; ComplexDouble += Double; signed-LHS with a same-or-narrower unsigned RHS. Same-width signed/unsigned in-place mixes follow the type_promote table by explicit maintainer ruling (2026-07-07): in-place and out-of-place stay consistent, and any future tightening happens in the table. The thrown message carries only the actionable core (both dtype names, the would-be result dtype, the out-of-place/astype() remedy); the provenance lives in the guard's comment block. PR #937 (credit: @ianmccul for the guard semantics and the original tests/Scalar_test.cpp) is superseded by this commit; its test suite is absorbed and extended here. Deliberate divergences from #937's broader guard, following the ruling and the Phase-3 plan: out-of-place binary arithmetic and comparisons never throw for real dtypes (they promote via type_promote; #937 disabled mixed signed/unsigned and Bool cases outright); in-place ops whose RHS-to-LHS conversion follows the type_promote table (e.g. Int64 += Int32) are allowed because the ruling's throw categories exclude them -- #937 had disabled any differing integer dtype; and integer abs()/sqrt() remain functional (#937 disabled them). Ordering comparisons on complex still throw; complex equality is allowed (as before). A known consequence of the table -- Scalar(Int64(-1)) == Scalar(Uint64 max) evaluating true via the Int64 promotion -- is pinned by test with maintainer sign-off. Storage_base::set_item(idx, const Scalar&) is re-implemented via std::visit on the Scalar's active alternative, forwarding the concrete value into the existing typed SetItem<T> (monostate throws instead of writing garbage). get_item already constructed the Scalar from the typed at<T>() and is unchanged. Migrated in-tree callers that relied on lossy in-place mixes (none were covered by existing tests; test tally is baseline + new, nothing removed): - BlockUniTensor::normalize_() and BlockFermionicUniTensor:: normalize_(): the norm^2 accumulator is now seeded with the floating dtype linalg::Norm() actually produces instead of this->dtype(), which for an integer-dtype UniTensor was an Int += Double lossy mix. The Norm output-dtype policy is centralized in a new constexpr Type_class::norm_result_dtype() (include/Type.hpp), used by both normalize_ sites and by src/linalg/Norm.cpp itself (replacing its hand-rolled enum-order dtype checks). - DenseUniTensor::Add_/Sub_ (dense += diagonal element loop): switched to out-of-place `v = v +/- rhs` since the two UniTensors' dtypes may legitimately differ (per #937's own migration guidance). - linalg_internal_cpu/{Add,Sub,Mul,Cpr}_internal.hpp: replaced the incidental Scalar_init_interface::type_promote_t alias (inherited from Type_class by the deleted PIMPL factory class) with Type_class::type_promote_t directly. - linalg_internal_cpu/Gemm_Batch_internal.hpp: replaced direct x._impl->to_cytnx_complex128()/64() with the public complex128()/ complex64() accessors. Tests: absorbs tests/Scalar_test.cpp (based on #937's, adjusted to the ruling's matrix) and extends it with a programmatic 11x11 dtype matrix over + - * / and comparisons (asserting result dtype == Type.type_promote(a,b) and value correctness), an 11x11 in-place throw matrix (throws iff type_promote(lhs,rhs) != lhs), and a pinned mixed-signedness equality quirk test. Adds pytests/scalar_variant_test.py covering Python arithmetic across dtypes, the new in-place RuntimeError behavior, Storage indexed assignment from Scalar, and numpy-scalar constructor paths (pinning a pre-existing pybind quirk where non-int64 integer numpy scalars resolve to the Int64 ctor overload). pybind/scalar_py.cpp is unchanged: none of its 84 bindings touched Scalar internals. Gates: C++ 1617 ran / 1602 passed (1086 baseline + 516 new) / 11 skipped / same 4 known linalg_Test.BkUt_* failures (#977). pytest 100 passed (57 baseline + 43 new). grep Scalar_base in include/ src/ pybind/ -> zero hits. build_py warning-clean. BREAKING CHANGE: mixed-dtype in-place Scalar arithmetic that would change or truncate the LHS dtype now throws cytnx::error (Python: RuntimeError) instead of silently truncating. Use out-of-place arithmetic or astype(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Scalar __init__ overload set registered the plain C++ integral constructors ahead of the py::numpy_scalar<T> ones. pybind11's plain arithmetic casters accept any __index__-capable object even in the no-convert pass (the trap documented in pybind/pyint_dispatch.hpp), so every in-range numpy integer scalar was consumed by the first plain integral overload: np.uint64/int32/uint32/int16/uint16 all came out dtype Int64. Plain Python ints fared no better: positive values hit the first-registered Uint64 constructor (Scalar(5) -> Uint64, and Scalar(True) -> Uint64, making Bool unreachable from a Python bool), and values beyond uint64 range were silently absorbed by the complex128 constructor's convert pass (Scalar(2**64) -> ComplexDouble). Rebind the constructor set in the canonical keep-set order from pybind/pyint_dispatch.hpp: numpy scalars first, then Python bool, then a single py::int_ overload that dispatches on magnitude via dispatch_pyint (int64 when it fits, uint64 otherwise, clean error beyond uint64 range), then double / complex128 (which absorb their np.float64 / np.complex128 subclasses). Also add the missing Scalar.__bool__ with numpy-consistent semantics (nonzero -> True; either component for complex). Besides truthiness, this fills the nb_bool slot that pybind11's bool caster requires, which unblocks Bool-storage round-trips through Scalar (storage[i] = Scalar(np.bool_(True)) previously raised "Unable to cast"). Flip the pytests that pinned the wrong dtypes (the former *_narrower_ints_preexisting_quirk test) to assert the correct ones, and cover the new keep-set and __bool__ behavior.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the cytnx::Scalar Python bindings to ensure correct constructor overload resolution for NumPy and Python scalar types, and introduces a NumPy-consistent __bool__ conversion operator. The review feedback correctly identifies a potential runtime error when evaluating the truthiness of an uninitialized or default-constructed Scalar (which has a Type.Void dtype) and suggests adding an explicit check to return false for void scalars, along with a corresponding test case.
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.
Comment on lines
+152
to
+159
| .def("__bool__", | ||
| [](const Scalar &s) { | ||
| if (Type.is_complex(s.dtype())) { | ||
| return static_cast<cytnx_double>(s.real()) != 0.0 || | ||
| static_cast<cytnx_double>(s.imag()) != 0.0; | ||
| } | ||
| return static_cast<cytnx_double>(s) != 0.0; | ||
| }) |
Contributor
There was a problem hiding this comment.
An uninitialized/default-constructed Scalar has Type.Void as its dtype. Currently, calling bool(Scalar()) will fall through to static_cast<cytnx_double>(s) != 0.0, which throws a runtime error (Cannot cast a Void (uninitialized) Scalar to any type.). Uninitialized/Void scalars should evaluate to False under boolean evaluation rather than throwing an exception. Adding an explicit check for Type.Void to return false resolves this issue.
.def("__bool__",
[](const Scalar &s) {
if (s.dtype() == Type.Void) {
return false;
}
if (Type.is_complex(s.dtype())) {
return static_cast<cytnx_double>(s.real()) != 0.0 ||
static_cast<cytnx_double>(s.imag()) != 0.0;
}
return static_cast<cytnx_double>(s) != 0.0;
})
Comment on lines
+314
to
+317
| def test_bool_complex_pure_imaginary_is_true(): | ||
| # numpy: bool(np.complex128(1j)) is True -- truthiness must consider the | ||
| # imaginary part, not just the real part. | ||
| assert bool(cytnx.Scalar(np.complex128(1j))) is True |
Contributor
There was a problem hiding this comment.
Add a test case to verify that a default-constructed/uninitialized Scalar (which has Type.Void dtype) evaluates to False under boolean conversion.
Suggested change
| def test_bool_complex_pure_imaginary_is_true(): | |
| # numpy: bool(np.complex128(1j)) is True -- truthiness must consider the | |
| # imaginary part, not just the real part. | |
| assert bool(cytnx.Scalar(np.complex128(1j))) is True | |
| def test_bool_complex_pure_imaginary_is_true(): | |
| # numpy: bool(np.complex128(1j)) is True -- truthiness must consider the | |
| # imaginary part, not just the real part. | |
| assert bool(cytnx.Scalar(np.complex128(1j))) is True | |
| def test_bool_void_scalar_is_false(): | |
| assert bool(cytnx.Scalar()) is False |
Collaborator
Code ReviewApplies the #991 keep-set discipline to Correctness — verified
Notes (non-blocking)
Clean, disciplined fix. LGTM. Posted by Claude Code on behalf of @pcchen |
pcchen
force-pushed
the
refactor/scalar-variant
branch
from
July 8, 2026 12:37
2f09a7d to
ea42698
Compare
Collaborator
Status update — base has moved; what the next rebase needs to handleThe review verdict above (LGTM) stands, but three things changed underneath this PR since it was written:
Also connected: #1011's CI is currently red on exactly the ctor-collapse quirk this PR fixes ( Standing from the review, unchanged: the Void Posted by Claude Code on behalf of @pcchen |
Collaborator
Author
|
superseded by comment in #1016 to drop Scalar python bindings. |
pcchen
added a commit
that referenced
this pull request
Jul 10, 2026
…oid guards, Sproxy consolidation Implements items B, C and D of the consolidated review checklist (A -- explicit constrained ctor + operator<=> -- is split into a follow-up PR, since it changes overload resolution codebase-wide and carries an unresolved complex-ordering semantic decision): B. Conversion surface (review threads on Scalar.hpp:354, Scalar.cpp:227/60/322): - One constrained template replaces the 9 per-type cast operators; conversions the language cannot perform error with Type_enum_name-based text (complex->real adds the real()/imag() hint). to_complex128()/to_complex64() delegate to it. - Type_enum_name<std::monostate> added (displays as "Void"). - astype() on a Void source now errors instead of silently staying Void; the dead convert_value/explicit_cast helpers are removed. C. Correctness items (codex findings): - conj() on a Void Scalar throws (old PIMPL contract) instead of silently returning Void. - Sproxy's 11 per-type assignment overloads collapse to one CytnxType-constrained template delegating through Scalar. - Type_class::norm_result_dtype(Void) rejects instead of mapping to Double. The CytnxType concept is the documented one-liner from #979. - Stale "lossy in-place combinations throw" comment updated to the promote semantics. D. pytest: test_inplace_same_dtype_keeps_dtype pins Int32 operands via astype() to sidestep the preexisting pybind ctor-collapse quirk that #1014 fixes (workaround documented for removal). Verified: C++ suite 1676 ran / 1665 passed / 11 skipped / 0 failed (4 new Void-guard tests); pytests/scalar_variant_test.py 43/43 against the freshly built extension; clang-format 14.0.6. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7 tasks
IvanaGyro
added a commit
that referenced
this pull request
Jul 15, 2026
Storage/LinOp/Scalar's keep-set consolidation dropped Scalar's raw per-C++-dtype py::init<>() constructors (cytnx_int64, cytnx_uint64, cytnx_int32, ...) in favor of the numpy_scalar-typed overloads plus a single py::int_ overload. Those raw constructors were the cause of a documented pre-existing quirk: a numpy scalar narrower than int64/ uint64 (np.uint64/int32/uint32/int16/uint16) always resolved to the raw Int64 constructor instead of its own numpy_scalar<T> overload, via the same "__index__ no-convert trap" documented under "KEEP-SET ORDERING" in pybind/pyint_dispatch.hpp. With the raw constructors gone, each of these five numpy dtypes now reaches its own overload and constructs a Scalar of the matching dtype (confirmed empirically: Scalar(np.uint64(5)).dtype() is now Uint64, not Int64, and likewise for int32/uint32/int16/uint16). Flips test_numpy_scalar_constructor_paths_narrower_ints_preexisting_ quirk from pinning the old Int64-for-everything behavior to asserting each numpy dtype's own correct dtype, and drops the now-stale "astype() is the reliable way to pin an unsigned dtype" rationale from test_inplace_signed_unsigned_mix_promotes_to_signed's comment (the direct numpy constructor now works, though astype() is kept for consistency with the surrounding tests in this file). This incidentally closes out the same behavior PR #1014 targeted (reviewed and approved, but closed as superseded once #1016 decided Scalar's Python binding should eventually be removed entirely, tracked in #1045 -- not yet started, so this fix has interim value). Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #1011 (
refactor/scalar-variant) — the fixed behavior is pre-existing on masterd6dcd160, but the regression tests that pinned the old behavior live in that branch'spytests/scalar_variant_test.py, so this lands on top of it and flips them in the same commit.Problem
scalar_py.cppregistered the plain C++ integral constructors ahead of thepy::numpy_scalar<T>__init__overloads. pybind11's plain arithmetic casters accept any__index__-capable object even in the no-convert pass — the exact trap documented under "KEEP-SET ORDERING" inpybind/pyint_dispatch.hpp(PR #991). Observed on the previous overload set:np.uint64/int32/uint32/int16/uint16(in range)Int64np.uint64(2**64-1)Uint64(int64 caster overflows, falls through)Uint64(unchanged)Scalar(5)/Scalar(2**63)Uint64/Uint64Int64/Uint64(magnitude dispatch)Scalar(2**64)ComplexDouble(complex ctor convert pass)Scalar(True)Uint64Boolbool(Scalar(np.int32(0)))True(no__bool__, default truthiness)Falsestorage[i] = Scalar(np.bool_(True))RuntimeError: Unable to castFix
bool(must precedepy::int_, which acceptsboolas anintsubclass), then a singlepy::int_overload dispatching on magnitude viadispatch_pyint, thendouble/complex128(absorbing theirnp.float64/np.complex128subclasses). Per the discipline, the per-C++-dtype integral constructors and the redundantnumpy_scalar<double>/numpy_scalar<complex<double>>overloads are dropped.Scalar.__bool__with numpy-consistent semantics (nonzero →True; either component for complex). This also fills thenb_boolslot pybind11's bool caster requires, unblocking Bool-storage round-trips through Scalar.Tests
TDD: tests flipped/added first and confirmed failing (21 red) on the unfixed build, then green after the reorder.
pytests/scalar_variant_test.py: the former*_narrower_ints_preexisting_quirktest now asserts correct dtypes insidetest_numpy_scalar_constructor_paths(all 11 dtypes); new coverage for uint64-above-int64-max value preservation, plain-int magnitude dispatch + out-of-range error, bool/float/complex constructor dtypes,__bool__truthiness across all dtypes, and the Bool row in the Storage round-trip.Full pytest suite against the dev build: 117 passed, 0 failed.
🤖 Generated with Claude Code