Fix remaining overload-cannot-match errors with the #915 keep-set strategy#1053
Merged
Conversation
![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 consolidates duplicate per-dtype overloads into unified 'keep-sets' across several pybind files (including LinOp, Scalar, Storage, Tensor, and UniTensor), resolving binding-level bugs and type-stub mismatches. The review feedback correctly identifies a critical bug in both dispatch_pyint_vector and from_pylist where negative integer overflows are incorrectly treated as requiring uint64_t conversion instead of triggering an out-of-range error, which would lead to invalid casts.
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.
IvanaGyro
commented
Jul 15, 2026
…loads pyint_dispatch.hpp's KEEP-SET ORDERING documents cytnx::Scalar as always registered last in each scalar-accepting operator's overload set. Scalar defines __float__/__complex__, so it structurally satisfies the typing.SupportsFloat and typing.SupportsComplex protocols that pybind11 3.0.4 renders for the cytnx_double/cytnx_complex128 overloads; with Scalar registered after those, mypy.stubtest correctly reports the Scalar overload as unreachable (overload-cannot-match), since any Scalar argument already matches an earlier, structurally broader overload in the stub's declared order. Move Scalar's overload to register immediately after py::int_, before cytnx_double/cytnx_complex128, in every scalar-accepting operator on Tensor and UniTensor. This only affects declaration order for pybind11's own overload resolution, which is independent of the mypy-visible order problem: pybind11 resolves a call in two passes (exact-type match, then conversion), and a plain Python float/complex/int is an exact match for cytnx_double/cytnx_complex128/py::int_'s casters in the first pass regardless of where Scalar sits, so Scalar's own implicit-conversion registrations (py::implicitly_convertible<cytnx_double, Scalar>() and the complex128 counterpart in scalar_py.cpp) are never reached for those arguments. Verified directly: float/complex/int/numpy-scalar/Scalar operands on __add__/__radd__/__sub__/__rsub__/__mul__/__rmul__/ __truediv__/__eq__ (Tensor) and __add__/__radd__/__iadd__ (UniTensor) all produce identical dtype and value before and after the reorder, and pytests/binding_dtype_test.py passes unchanged (18/20; the other 2 hit an unrelated pre-existing ASan __cxa_throw interceptor issue on exception-raising __bool__ tests, reproducible on the documented build workflow independent of this change). Co-Authored-By: Claude <noreply@anthropic.com>
Two gaps left over from the earlier Scalar-reorder pass and #915's keep-set consolidation, both still on Tensor: - fill/append were still bound once per raw C++ dtype (complex128, complex64, double, float, int64, uint64, int32, uint32, int16, uint16, bool), duplicating the same overload-cannot-match problem #915/ecb1fbd already fixed for the arithmetic operators. Replaced with the same numpy_scalar/py::int_/double/complex128 keep-set. - __iadd__, __isub__, __imul__, __itruediv__, __ifloordiv__, __ne__, and __setitem__ still had their Scalar overload registered after cytnx_double/cytnx_complex128, unreachable in the stub for the same structural-subtyping reason as the operators already fixed (Scalar defines __float__/__complex__, so it satisfies the SupportsFloat/ SupportsComplex protocols those overloads render as). These use a py::object self + multi-line lambda body shape that the original scan (looking for `[]` immediately after the comma) didn't match, so they were missed. Reordered Scalar to register right after py::int_, before cytnx_double/cytnx_complex128, matching every other operator. Co-Authored-By: Claude <noreply@anthropic.com>
Storage.fill/append, LinOp.set_elem, and Scalar's constructor each had two coexisting registration styles: a raw per-C++-dtype set (complex128, complex64, double, float, int64, uint64, int32, uint32, int16, uint16, bool) and, for LinOp/Scalar, an already-correct numpy_scalar-wrapped set added later but never cleaned up. Replaced with a single keep-set per method, dropping the narrow-width duplicates the numpy_scalar overloads already cover. The numpy_scalar block must be registered before any plain integral/ double/complex128 overload (see "KEEP-SET ORDERING" in pybind/pyint_dispatch.hpp): a numpy scalar satisfies __index__/ __float__/__complex__, so an earlier plain overload can consume it in pybind11's conversion pass for any numpy dtype the keep-set omits (e.g. float16, which none of these three keep-sets register a numpy_scalar overload for). LinOp.set_elem and Scalar's raw registrations were ordered the other way around; reordered for consistency with every other keep-set in this codebase, not just to remove the stub-visible duplicate. LinOp.set_elem's target assignment (`x(0) = elem`, via Tensor::Tproxy::operator=) tolerates a complex-into-real narrowing that Tensor's own set<T>()/__setitem__ path rejects, so no live crash was reproduced for LinOp specifically -- this is a defensive consistency fix, not a confirmed bug fix. Scalar's constructor drops separate uint64/int64 overloads (which rendered as the identical stub annotation regardless of order, so one was always flagged unreachable even though a small positive value vs. a negative one previously exercised each of them respectively, purely from uint64 being registered first) in favor of a single py::int_ overload with dispatch_pyint. This adopts dispatch_pyint's int64- preferred convention, already used by every other keep-set in this codebase, for consistency; Scalar(5).dtype() changes from Uint64 to Int64. Storage.from_pylist gets the same treatment plus a fix for a live bug: Python bool is an int subclass, and with Storage.from_vector<cytnx_bool> registered after from_vector<cytnx_uint64> (its previous position), Storage.from_pylist([True, False]) silently produced a Uint64-dtype Storage instead of Bool. Reordering bool first, and moving double ahead of complex128 for the same covariant-Sequence reason as the numpy scalars (Sequence[SupportsFloat | SupportsIndex] is a subtype of Sequence[SupportsComplex | SupportsFloat | SupportsIndex]), fixes the reachability rather than just the stub duplicate. The int-list case becomes a single py::sequence overload that inspects every element's magnitude and picks int64 unless any element needs uint64's range, matching dispatch_pyint's convention (Storage.from_pylist([1, 2, 3]) was Uint64, is now Int64) instead of two overloads that rendered as the identical stub annotation. Storage.__len__ was registered twice, identically; dropped the duplicate. Co-Authored-By: Claude <noreply@anthropic.com>
Helpclass.set_elem (the UniTensor.at() proxy's element setter) had the same raw-per-C++-dtype-plus-unused-numpy_scalar-block duplication as Storage/LinOp's set_elem/fill/append, fixed the same way. UniTensor.get_block/get_block_ each had separate vector<int64>/ vector<uint64> qnum overloads, which rendered as the identical stub annotation regardless of registration order (same issue as Storage.from_pylist's int-list case), so one was always flagged overload-cannot-match even though both were reachable depending on list content. Replaced with a single py::sequence overload using a new dispatch_pyint_vector helper (the vector counterpart of pyint_dispatch.hpp's dispatch_pyint), which picks int64 unless any element needs uint64's range. Symmetry.clone and UniTensor.get_blocks_ were each registered twice, identically; dropped the duplicates. Co-Authored-By: Claude <noreply@anthropic.com>
The comments added for LinOp.set_elem, Scalar's constructor,
Storage.from_pylist, and UniTensor's dispatch_pyint_vector described what
the previous registration produced ("was Uint64, is now Int64", "matched
the raw complex128 overload... before this change"). That narrative
belongs in the commit message, not in a comment describing the code as it
stands; a comment describing a prior state ages badly once nobody
remembers what "previously" refers to. Trimmed each comment to describe
only the current registration order and why it matters, and let
clang-format correct dispatch_pyint_vector's indentation inside its
enclosing anonymous namespace while the file was open.
Tensor defines __iter__, so numpy's ufunc dispatch machinery treats a numpy-scalar-on-the-left operand (e.g. np.float32(1.0) + t) as array-like and tries to iterate it before ever falling back to __radd__/__rsub__/__rmul__/__rtruediv__/__rfloordiv__/__rmod__, raising "TypeError: 'TensorIterator' object is not iterable" (issue #692) regardless of numpy scalar dtype -- confirmed empirically across int64, complex64, uint16, and bool. The numpy_scalar<...>-typed overloads registered for these six operators were therefore never reachable, and mypy.stubtest flags them as [misc] "Forward operator not callable" under Python 3.10's numpy 2.2.6 (numpy's own typeshed limitation, not fixable from cytnx's side). Removed the nine numpy_scalar overloads per operator; the py::int_/Scalar/double/ complex128 overloads are unaffected, since a plain Python operand never goes through numpy's ufunc dispatch and the stub for those still matches reality. UniTensor's equivalent reverse operators are reachable (confirmed empirically for np.int64/np.float32 on the left) and are not touched. Co-Authored-By: Claude <noreply@anthropic.com>
Covers the dtype-selection bugs fixed by consolidating per-C++-dtype duplicate pybind11 overloads into keep-sets: Storage.from_pylist mis-selecting Uint64 instead of Bool for a list of Python bools (bool is an int subclass) and preferring Uint64 over Int64 for a plain non-negative int list purely from registration order; Scalar's plain-int constructor's analogous Uint64/Int64 tie-break. Also adds light reachability coverage for UniTensor.get_block/get_block_'s collapsed qnum-list overload, LinOp.set_elem's numpy-scalar keep-set, and Tensor's __r*__ operators (numpy-scalar-on-the-left still raises the same TypeError as before; the remaining py::int_/Scalar/double/ complex128 overloads still work). Co-Authored-By: Claude <noreply@anthropic.com>
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>
Storage.from_pylist's int-list dispatch and UniTensor's dispatch_pyint_vector (used by get_block/get_block_'s qnum parameter) both used PyLong_AsLongLongAndOverflow's overflow flag to decide between an int64 and a uint64 kernel, treating any nonzero overflow the same way. PyLong_AsLongLongAndOverflow sets overflow to -1 for a value below LLONG_MIN, not just +1 for a value above LLONG_MAX -- a value that negative does not fit in uint64_t either (uint64_t cannot represent any negative value), so routing it into the uint64 branch led to a failed cast surfacing as a generic pybind11 RuntimeError instead of a clean domain error. Split the check: overflow < 0 raises immediately via cytnx_error_msg, overflow > 0 sets the uint64 flag, matching pyint_dispatch.hpp's single-value dispatch_pyint (which already handles this correctly via PyLong_AsUnsignedLongLong's own OverflowError). Also stopped breaking out of the scan on the first overflow, so a negative overflow later in a list is still caught even if an earlier element needed uint64. Co-Authored-By: Claude <noreply@anthropic.com>
Three fixes to review comments on this file: - The negative-overflow tests for Storage.from_pylist and UniTensor.get_block only asserted on the exception type (cytnx.CytnxError), which any unrelated validation error (e.g. a qnum mismatch) would also satisfy, so the tests did not actually discriminate the overflow-specific code path from other failure modes. Added match= on the specific "out of the supported int64/uint64 range" message. - test_linop_set_elem_numpy_scalar_dtypes asserted a computed value that would still match even if np.float32(1.5) were silently routed through the wrong (complex128) overload -- LinOp.set_elem's target assignment tolerates that without raising or changing the resulting value, so no assertion on the result can distinguish which overload matched. Renamed to test_linop_set_elem_numpy_scalar_dtypes_are_ accepted and reworded the section comment to state plainly that this is reachability coverage only, not a discriminating regression test. - test_tensor_numpy_scalar_on_left_still_raises_typeerror pinned issue #692's still-unreachable numpy-scalar-on-the-left behavior as an expected TypeError. #692 tracks this as a genuine bug, not a design decision, and a test asserting a known bug's current symptom needs updating (and could be forgotten) the moment #692 is fixed. Removed it; the regression coverage that matters for this PR is that the kept overloads still work, which test_tensor_plain_python_and_scalar_reverse_ops_still_work already covers. Co-Authored-By: Claude <noreply@anthropic.com>
Fresh python tools/generate_stubs.py run against the fully-rebased source tree (fix/overload-cannot-match-remaining replayed onto claude/fix-stubtest-errors), superseding the two stub-regeneration commits from before the rebase whose generated content was superseded by intervening changes on the new base. Co-Authored-By: Claude <noreply@anthropic.com>
IvanaGyro
force-pushed
the
fix/overload-cannot-match-remaining
branch
from
July 15, 2026 14:00
70b0e59 to
1df8cbe
Compare
IvanaGyro
marked this pull request as ready for review
July 15, 2026 14:02
![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: 1df8cbe608
ℹ️ 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".
from_pylist's Bool overload bound Storage::from_vector<cytnx_bool> directly, which pybind11 exposes through the arithmetic bool type_caster. That caster's convert-pass falls back to PyObject_IsTrue() for any object, so a list of numpy scalar elements narrower than int64/uint64/float64/complex128 (e.g. np.int32, np.uint16, np.float32) matched nothing in the no-convert pass and then matched the Bool overload's convert pass by truthiness -- from_pylist([np.int32(2)]) silently produced a Bool-dtype Storage holding True instead of an Int32 value of 2. Fixes this two ways: - Add a numpy_scalar keep-set (complex64/float/int64/uint64/int32/ uint32/int16/uint16/bool) ahead of the Bool overload, per the KEEP-SET ORDERING convention in pyint_dispatch.hpp, so these numpy dtypes match their own overload in the no-convert pass before the Bool overload's convert pass is ever attempted. - Replace the Bool overload's binding with a lambda over std::vector<py::bool_>. py::bool_'s caster does a strict isinstance check with no truthiness-based convert-pass fallback (unlike the arithmetic bool caster), so any remaining non-bool input now falls through to "no matching overload" instead of being silently misclassified. A numpy scalar type narrower than float64/complex128 that has no cytnx-backed dtype at all (e.g. np.float16, np.int8) still widens to Double via the existing double overload, matching that overload's existing catch-all role documented in the keep-set comment -- it does not raise.
Covers the dtype and value produced by from_pylist for each of the numpy scalar types that previously fell through to the Bool overload's truthiness-based misclassification (int64/uint64/int32/uint32/int16/ uint16/float32/complex64/bool).
Adds the 9 new from_pylist overloads to __init__.pyi. Also corrects linalg.pyi's ExpH/ExpM b parameter default from the elided "..." repr back to "0j" -- the prior stub-regeneration commit ran against a stale build/debug-openblas-cpu-venv .so that predated the ComplexArg fix, so it didn't pick up the corrected default repr despite the C++ side already having it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76e6f4246b
ℹ️ 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".
The qnum consolidation replaced get_block/get_block_'s raw vector<cytnx_int64>/vector<cytnx_uint64> overloads with a single std::vector<py::int_> dispatcher (dispatch_pyint_vector). The removed overloads used pybind11's arithmetic integral caster, which accepts any object satisfying __index__ (numpy integer scalars included) even in the no-convert pass; py::int_'s caster does a strict isinstance(int) check with no such fallback, so numpy integers are not Python int subclasses and matched neither overload. get_block([np.int64(0), np.int64(0)], False) went from working to raising TypeError. Adds a dispatch_numpy_int_vector helper and a numpy_scalar keep-set (int64/uint64/int32/uint32/int16/uint16) ahead of the py::int_ dispatcher, for both the labeled and unlabeled get_block/get_block_ forms, mirroring the same fix already applied to Storage.from_pylist.
Covers the labeled and unlabeled get_block/get_block_ forms across the six numpy integer widths that previously stopped matching after the qnum consolidation replaced their raw vector<cytnx_int64>/ vector<cytnx_uint64> overloads with a stricter py::int_ dispatcher.
Adds the 24 new get_block/get_block_ overloads (4 call sites x 6 numpy integer widths) to __init__.pyi.
get_block/get_block_'s vector argument is the per-leg quantum-number
sector index, matched against BlockUniTensor's _inner_to_outer_idx
after an unsigned cast (include/UniTensor.hpp: "this one for Block
will return the indicies!!"), not the physical charge value. The test
names and one comment claimed otherwise ("U1 does support negative
charges physically"), which doesn't apply to this API -- get_block
never receives a charge value in the first place. Renamed the
qnum-suffixed test names and qnum-named locals to qindex, and
rewrote the negative-overflow test's comment to describe it as a
mechanical dispatch_pyint_vector check rather than a claim about
negative charges.
The public py::arg("qnum") name in pybind/unitensor_py.cpp is
unchanged -- it predates this PR and renaming a public keyword
argument is a separate, breaking API change.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25166e02b1
ℹ️ 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".
cytnx has no Int8/Uint8 dtype, so the numpy_scalar keep-set added to Scalar's constructor had no int8/uint8 entry. Without one ahead of the raw cytnx_double constructor, np.int8/np.uint8 fell through to it -- Scalar(np.int8(5)).dtype() silently became Double instead of an integer dtype. Before this PR's constructor consolidation, these values were incidentally caught by a raw arithmetic-typed constructor registered ahead of the numpy_scalar block (pybind11's arithmetic integral caster accepts anything with __index__ even in the no-convert pass), landing on some integer dtype; this made the regression floating-point where it used to be integer, not merely a different integer width. Widens to Int16/Uint16, the narrowest integer dtype cytnx supports, rather than a real-valued dtype. The same numpy_scalar<int16_t>/<uint16_t> keep-set pattern (with no int8/uint8 counterpart) appears in ~90 other sites across tensor_py.cpp, unitensor_py.cpp, storage_py.cpp, and linop_py.cpp; fixing all of them is out of scope here and tracked in #1065.
Covers dtype and sign for np.int8/np.uint8 constructor arguments, which previously fell through to Scalar's raw cytnx_double constructor and produced a floating-point dtype instead of an integer one.
Adds the 2 new Scalar.__init__ overloads to __init__.pyi.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb0cb2b117
ℹ️ 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".
Same gap as Scalar's constructor (#1053): cytnx has no Int8/Uint8 dtype, so from_pylist's numpy_scalar keep-set had no int8/uint8 entry and those inputs fell through to the double overload -- from_pylist([np.int8(5)]) produced a Double storage instead of an integer one. Widens to Int16/Uint16, matching the Scalar fix. Also documents (without attempting to fix by reordering) that from_pylist([]) changed its default dtype from ComplexDouble to ComplexFloat as a side effect of this keep-set's numpy_scalar-first ordering: an empty list has no elements for any vector<T> caster to check, so every from_pylist overload matches it equally and the first-registered one wins by construction. Moving complex128 first to preserve the old default was tried and reverted -- mypy's overload checker has no notion of pybind11's runtime no-convert/convert passes, so an earlier "complex" stub signature unconditionally shadows every later, narrower signature, reintroducing 14 overload-cannot-match errors (exactly the class of bug this PR fixes). No overload ordering satisfies both constraints (numpy_scalar-first for non-empty-list correctness, complex128-first for the old empty-list default) simultaneously under pybind11's per-dtype-overload model.
… dtype Covers dtype and value for np.int8/np.uint8 elements (previously fell through to Double), and pins from_pylist([])'s new ComplexFloat default (previously ComplexDouble) as a regression test for the default itself -- see storage_py.cpp's from_pylist comment for why no overload ordering can restore the old default without breaking mypy.stubtest.
Adds the 2 new from_pylist overloads to __init__.pyi.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6ca1ee74c
ℹ️ 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".
| py::arg("pylist"), py::arg("device") = (int)cytnx::Device.cpu) | ||
| .def_static( | ||
| "from_pylist", | ||
| [](const std::vector<py::int_> &pylist, const int &device) { |
There was a problem hiding this comment.
Fresh evidence beyond the prior homogeneous NumPy-scalar fixes: a sequence like Storage.from_pylist([np.int32(2), 3]) has no integer overload that can accept every element, because the exact std::vector<py::numpy_scalar<int32_t>> overload above rejects the Python int and this std::vector<py::int_> overload rejects the NumPy scalar. The call then reaches the cytnx_double vector overload and creates a Double storage, whereas the removed raw integral vector overloads accepted both values via __index__. Please dispatch integer sequences element-by-element, or otherwise accept generic __index__ values, before the double/complex fallbacks.
Useful? React with 👍 / 👎.
| [](UniTensor &self, const std::vector<std::string> &labels, const std::vector<cytnx_int64> &qnum, const bool &force) { | ||
| return self.get_block_(labels, qnum, force); | ||
| .def("get_block", | ||
| [](const UniTensor &self, const std::vector<py::int_> &qnum, const bool &force) { |
There was a problem hiding this comment.
Fresh evidence beyond the prior homogeneous NumPy qindex fix: get_block([np.int32(0), 0], False) still has no matching overload, since the exact std::vector<py::numpy_scalar<int32_t>> overload above rejects the Python literal and this std::vector<py::int_> overload rejects the NumPy scalar. The removed vector<cytnx_int64>/vector<cytnx_uint64> overloads accepted mixed __index__ values, so callers that combine NumPy qindices with Python literals now get TypeError; dispatch the qnum sequence element-by-element or accept a generic __index__ sequence.
Useful? React with 👍 / 👎.
An empty Python list has no elements for any of from_pylist's vector<T> overloads to check, so pybind11's no-convert pass accepts it for every one of them equally and the first-registered overload wins regardless of T -- from_pylist([]) was at the mercy of registration order alone, and produced ComplexFloat. Reordering the overload set to control which one wins the empty case was tried (see the previous commit's from_pylist comment) and reverted: it conflicts with keeping complex128 registered last, which mypy.stubtest requires. Handled instead with an explicit pylist.empty() check inside the first-registered overload's body. This only changes the function's runtime behavior, not its declared parameter type, so it doesn't touch the stub-visible signature or interact with mypy.stubtest at all. Routes to Double, matching numpy/the Python array API's own empty-array, no-explicit-dtype default (np.array([]).dtype == float64) instead of the arbitrary ComplexFloat this keep-set's ordering happened to produce.
…ault Updates the empty-list regression test for the previous commit's fix (was pinned to the arbitrary ComplexFloat the keep-set's registration order happened to produce; now pinned to Double, matching np.array([]).dtype).
Member
Author
|
I am tired on keeping this PR on tip of the master branch. I have maintained this stub change for weeks. During these weeks, many PRs merged into master, which cause me to update the branch. Please take over this PR and the following PRs. I am stopping my AI bot tracing on these PRs. |
Collaborator
I will merge this PR but I suggest that we postpone the stub update until we stabilize the code base. |
yingjerkao
approved these changes
Jul 17, 2026
Brings #1053 up to date with master after #915 landed. Only two files conflicted: - pybind/tensor_py.cpp: master (#941) deliberately UNBINDS Tensor's __floordiv__/__rfloordiv__/__ifloordiv__ (binding them to Div would make `t // x` silently perform true division). Master's own comment states this "supersedes the P1-T2/T3 keep-set treatment these operator groups received" -- i.e. it directly supersedes #1053's floordiv keep-set. Resolved by taking master's unbind; #1053's keep-set consolidation for all other operators (add/sub/mul/truediv/mod/eq, plus Storage from_pylist and UniTensor/Scalar/LinOp/Symmetry) is preserved. #1053's keep-set tests do not exercise floordiv, so nothing of #1053 breaks. - cytnx/cytnx/__init__.pyi: regenerated from the merged bindings via tools/generate_stubs.py (the correct resolution for generated output), reverting only a machine-specific Device.pyi Ncpus value. Testing (openblas-cpu): pycytnx builds; mypy.stubtest reports 0 overload-cannot-match (down from 535 pre-#1053), leaving only the pre- existing 7 override + 5 assignment __eq__/__hash__ warnings (a separate, out-of-scope class); pytest pytests/ = 308 passed, 1 skipped. 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: 61b7aa5ffd
ℹ️ 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".
Comment on lines
+1245
to
+1249
| "get_block", | ||
| [](const UniTensor &self, const std::vector<py::numpy_scalar<uint16_t>> &qnum, | ||
| const bool &force) { | ||
| return dispatch_numpy_int_vector(qnum, | ||
| [&](const auto &v) { return self.get_block(v, force); }); |
There was a problem hiding this comment.
When callers pass qindices as np.int8/np.uint8 scalars or arrays, they no longer match any of these homogeneous py::numpy_scalar<...> overloads, and the std::vector<py::int_> fallback below rejects NumPy scalar elements. The removed raw vector<cytnx_int64>/vector<cytnx_uint64> overloads accepted those values via __index__, so e.g. a small np.array([0, 0], dtype=np.int8) now raises TypeError for get_block/get_block_ instead of selecting the block; add int8/uint8 qindex handlers before the py::int_ fallback.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1053 +/- ##
==========================================
+ Coverage 72.29% 72.64% +0.35%
==========================================
Files 226 226
Lines 27944 28184 +240
Branches 71 71
==========================================
+ Hits 20202 20475 +273
+ Misses 7721 7688 -33
Partials 21 21
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
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
3 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.
Problem
mypy.stubtestreported 768overload-cannot-matcherrors across the pybind11 bindings (Tensor,UniTensor,Storage,Scalar,LinOp,Symmetry) after PR #915 fixed the same class of error forExpH/ExpM. The root cause was the same in every case: a method registered a raw.def(...)overload per C++ dtype (11+ per operator) alongside — or ahead of — an already-correctnumpy_scalar-typed keep-set, so pybind11-stubgen emitted duplicate or identically-rendered@typing.overloadsignatures that mypy flags as unreachable. A few of these coexisting raw registrations were also ordered before theirnumpy_scalarcounterpart, which is a live reachability bug (documented as the "__index__no-convert trap" / "__float__-fallback trap" inpybind/pyint_dispatch.hpp), not just a stub-quality issue.Fix
Applied #915's keep-set consolidation (one overload per Python-distinguishable dtype,
numpy_scalarblock first, thenpy::int_/Scalar/double/complex128) across the remaining binding surface:Tensor/UniTensor: collapsedfill/append/set_elem/arithmetic-operator overload sets; reorderedScalarahead ofdouble/complex128where it had been registered after (verified this does not change pass-1 overload resolution for plain Python literals —Scalar'simplicitly_convertibleregistrations only take effect in pybind11's convert pass).Storage: same treatment forfill/append/from_pylist.from_pylisthad several live bugs, found across multiple review passes:Storage.from_pylist([True, False])silently producedUint64instead ofBool(Pythonboolis anintsubclass, andfrom_vectorwas registered after theuint64overload). Fixed by reordering.Storage.from_pylist([np.int32(2)])(and other numpy scalar dtypes narrower than int64/uint64/float64/complex128) silently produced aBoolstorage holdingTrueinstead of the numeric value — numpy integer/bool scalars aren't subclasses of Python int/bool, so they matched nothing in pybind11's no-convert pass and fell through to the Bool overload's convert-pass truthiness fallback. Fixed by adding anumpy_scalarkeep-set ahead of the Bool overload, and making the Bool overload itself strict (std::vector<py::bool_>, no truthiness fallback).Storage.from_pylist([np.int8(5)])producedDoubleinstead of an integer dtype (cytnx has noInt8/Uint8; the new keep-set had no entry for them). Fixed by widening toInt16/Uint16.Storage.from_pylist([])'s dtype default is now handled by an explicitpylist.empty()check in the first-matching overload's body, matching numpy/the Python array API's own convention (np.array([]).dtype == float64) — a runtime-only change that doesn't touch any stub-visible signature. (Reordering the overload set to control the empty-list winner was tried first and reverted: it conflicts with keepingcomplex128registered last, whichmypy.stubtestrequires.)LinOp.set_elem: same duplicate-set cleanup; the numpy_scalar block must precedepy::int_/double/complex128, matching every other keep-set in the codebase.Scalar.__init__: dropped the separateuint64/int64raw constructors (which rendered as an identical stub signature) for a singlepy::int_overload viadispatch_pyint. This is also a live bug fix: those raw constructors were intercepting every numpy int scalar narrower than int64 (np.uint64/int32/uint32/int16/uint16) via the same no-convert trap, so e.g.Scalar(np.uint64(5)).dtype()wasInt64instead ofUint64— this incidentally closes out the same behavior a previously-approved-but-superseded PR (fix(pybind): keep-set ordering for Scalar constructors, add __bool__ #1014) targeted.Scalar(5).dtype()also changes fromUint64toInt64(magnitude-based dispatch, matching every other keep-set's int64-preferred convention).Scalar(np.int8(5))had the same "no Int8 dtype" gap asfrom_pylistabove (wasDouble, now widens toInt16). Note:Scalar's Python binding is slated for eventual removal (Remove the cytnx.Scalar Python binding (Python should derive type from dtype) #1045, not yet started) per the maintainers' direction in Design question: should scalars be rank-0 (shape-{}) tensors? #1016 — these fixes have interim value and don't block that removal.UniTensor.get_block/get_block_: collapsed the qnum parameter's separatevector/vectoroverloads (identical stub signature) into onevectoroverload via a newdispatch_pyint_vectorhelper. This introduced its own regression: the removed rawvector<cytnx_int64>/vector<cytnx_uint64>overloads accepted numpy integer scalars through pybind11's arithmetic-type__index__path, but the replacementpy::int_-based dispatcher only accepts actual Pythonint—get_block([np.int64(0), np.int64(0)], False)regressed from working to raisingTypeError. Fixed by adding anumpy_scalarkeep-set (int64/uint64/int32/uint32/int16/uint16) ahead of thepy::int_dispatcher, for both the labeled and unlabeled forms.Tensor.__radd__/__rsub__/__rmul__/__rtruediv__/__rfloordiv__/__rmod__: removed thenumpy_scalar-typed overloads entirely.Tensordefines__iter__, so numpy's ufunc dispatch machinery treats a numpy-scalar-on-the-left operand as array-like and tries to iterate it before ever falling back to these reverse operators, raisingTypeError: 'TensorIterator' object is not iterableregardless of dtype (issue Unexpected dtype returned when operating with Float32 / ComplexFloat32 tensors and numpy scalars #692) — confirmed empirically across int64/complex64/uint16/bool, so the overloads were dead code and also the source ofmypy.stubtest [misc]"Forward operator not callable" errors under Python 3.10's numpy 2.2.6 (a numpy typeshed limitation, not fixable from cytnx's side).UniTensor's equivalent reverse operators are reachable and untouched.Storage.__len__,Symmetry.clone,UniTensor.get_blocks_) were dropped.tools/generate_stubs.py: added acomplex |prefix to theSupportsComplex | SupportsFloat | SupportsIndexunion pybind11 3.0.4 emits forcytnx_complex128parameters —typing.SupportsComplexalone rejects a plain Pythoncomplexliteral sincecomplexhas no__complex__.Regenerated
cytnx/cytnx/*.pyi(Python 3.10, perCONTRIBUTING.md's recipe) against all of the above, including a rebase onto #915's ownExpH/ExpMfix (a dedicated binding-levelComplexArgtype_caster inpybind/complex_arg.hpp, added after this PR branched).Scope notes:
chatgpt-codex-connectorreview passes found the same "no Int8/Uint8 dtype in the numpy_scalar keep-set" gap repeated in ~90 other sites acrosstensor_py.cpp,unitensor_py.cpp, andlinop_py.cppthat predate this PR and weren't otherwise touched by it. Fixed where the gap was in code this PR itself introduced (Scalar.__init__,Storage.from_pylist); the rest is tracked in Keep-set numpy_scalar overloads (PR #1053) don't cover np.int8/np.uint8, silently widening to a real dtype #1065 rather than expanding this PR's scope.from_pylist([np.int32(2), 3]),get_block([np.int32(0), 0], False)) has no matching overload under this per-dtype-overload model, since pybind11'sstd::vector<T>requires every element to satisfy the same caster. Fixing this needs a larger rewrite (a single manualpy::sequence-based dispatcher replacing the whole numpy_scalar+int keep-set) — tracked in Storage.from_pylist/UniTensor.get_block reject mixed Python-int/numpy-scalar sequences (and can't preserve the old empty-list default) under the per-dtype overload model #1066 rather than attempted here.Testing
python -m mypy.stubtest cytnx.cytnx(Python 3.10): 0overload-cannot-matcherrors (down from 768). Remaining[misc](73),[assignment](5),[override](7) errors are pre-existing pybind11-stubgen__eq__/__hash__Liskov-substitution boilerplate, unrelated to this PR.ctest --test-dir build/debug-openblas-cpu/build/debug-mkl-cpu: 1767/1768 passed both presets — the one failure (BlockUniTensorTest.CombineBondForceDoesNotRewriteUserHeldBond) is a pre-existing, unrelated ASanmemcpy-param-overlapbug insrc/BlockUniTensor.cpp/src/utils/vec_map.cpp(this PR touches no files undersrc/,include/, ortests/), filed as ASan memcpy-param-overlap in BlockUniTensor::permute_ (via combineBond), CombineBondForceDoesNotRewriteUserHeldBond #1052.pytest pytests/: 288 passed, 1 skipped (pre-existing, environment-dependent graphviz skip).pytests/keep_set_dispatch_test.pyfor the dtype-selection bugs fixed here (Storage.from_pylist's bool/int/numpy-scalar/8-bit/empty-list dispatch,Scalar's plain-int and 8-bit constructor dispatch,LinOp.set_elem's numpy-scalar dispatch,UniTensor.get_block's qindex dispatch including numpy integer widths, negative-overflow rejection), and flippedpytests/scalar_variant_test.py's narrower-int constructor test from pinning the old buggy behavior to asserting the fixed one.Closes out the remaining scope of #928 (stub-quality fixes at the binding layer instead of
generate_stubs.pypost-processing) for theoverload-cannot-matchcategory specifically.