Bind ExpH/ExpM with per-dtype overloads + Python-layer fixes (toward clean stubtest) - #915
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 simplifies and consolidates the Python bindings for ExpH and ExpM by replacing numerous type-specific overloads with a single overload accepting cytnx::Scalar, supported by implicit conversions for NumPy scalars and a C++ dispatch helper. It also cleans up unused imports, refactors type stubs, and adds unit tests for the new scalar handling. The review feedback highlights two key improvement opportunities: first, the return_indices parameter in getDegeneracy is currently ignored and should be respected; second, py::numpy_scalar<bool> should be registered for implicit conversion to Scalar to prevent runtime type errors with NumPy boolean scalars.
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.
![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: 6f1f3cc9d3
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fb440de6f
ℹ️ 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".
|
I didn't really know what mypy's stubtest tests and tlthe best solution either. This PR may not correct. |
94d54c9 to
bb32d2d
Compare
c7473d7 to
6700547
Compare
bb32d2d to
5bd01bb
Compare
IvanaGyro
changed the title
|
Heads-up from running the full
This is unrelated to the changes here: TDVP drives time evolution through — Claude Generated by Claude Code |
6700547 to
bffc010
Compare
25a5894 to
55031b7
Compare
bffc010 to
c1e1f45
Compare
55031b7 to
f182481
Compare
@claude: When examining failed tests, do not start by questioning the tolerance value. You should first inspect carefully the tested code. In this case, the TDVP test is a trivially local imaginary-time evolution problem that should, in principle, converge to essentially numerical precision. In fact the @ Humans: Recommend putting above advice into local CLAUDE.md or other AI-agent instructions. I note that the PR branch has since been rebased onto current master. In this case it is essentially a rediscovery of #888. After #889 and #892 that test passes with tolerance around 4.21e-8. And that is primarily due to the relatively short imaginary time evolution. So actually the tolerance is weaker than it needs to be, and it isn't yet a sharp test of the numerics. |
|
If the goal is to bind It does not make much sense to split the Python binding by dtype, immediately fold those overloads back into a runtime-selected Cytnx dtype in C++, and then dispatch again at runtime to the backend implementation. That keeps the fragile runtime dtype path in the middle of the call chain. We have already seen with As a general design rule, if the Python binding needs one overload per dtype, then the C++ side should also have strongly typed overloads or template instantiations for those dtypes. The binding should dispatch into those typed C++ functions directly, rather than funneling back through a runtime dtype and an untyped backend dispatch path. |
725a8dd to
37e3210
Compare
6832f72 to
bab016c
Compare
37e3210 to
9e7e777
Compare
3088d86 to
247f104
Compare
1e9b842 to
262d3d3
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #915 +/- ##
==========================================
+ Coverage 72.11% 72.29% +0.18%
==========================================
Files 225 226 +1
Lines 28207 27944 -263
Branches 71 71
==========================================
- Hits 20341 20202 -139
+ Misses 7845 7721 -124
Partials 21 21
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
@pcchen May you help to review this? |
Several bindings changed on master without regenerating the committed cytnx/cytnx/*.pyi stubs, so they drifted from what the pinned pybind11 3.0.4 / pybind11-stubgen 2.5.5 toolchain actually emits. Regenerate them from a clean build so the committed stubs match the current extension before layering further binding changes on top. Co-Authored-By: Claude <noreply@anthropic.com>
linalg.ExpH and linalg.ExpM were bound once per C++ scalar dtype -- the native cytnx_* types plus py::numpy_scalar<...> variants, roughly twenty overloads per function across the UniTensor and Tensor signatures. pybind11-stubgen collapses several distinct C++ types onto the same Python annotation, so the generated stub carried duplicate @typing.overload signatures that mypy rejects with overload-cannot-match. Keep one binding per Python-distinguishable dtype and drop the redundant ones: - A Python float/complex already binds to the cytnx_double/cytnx_complex128 overloads, and np.float64/np.complex128 are builtin subclasses caught by those same overloads, so the cytnx_float, cytnx_complex64, numpy_scalar<double> and numpy_scalar<complex<double>> overloads were unreachable duplicates. - The fixed-width integer overloads all accept the same Python int, so replace them with a single py::int_ overload that dispatches on magnitude to the int64 or uint64 kernel. Python int is arbitrary precision, which no single fixed-width C++ overload can cover, so the runtime branch keeps the whole range behind one signature. Under pybind11 3.0.4 the kept overloads render as the numpy.* scalar types, `int` (py::int_), `typing.SupportsFloat | typing.SupportsIndex` (cytnx_double), and `typing.SupportsComplex | typing.SupportsFloat | typing.SupportsIndex` (cytnx_complex128). The complex128 annotation is a supertype of the cytnx_double and integer ones, but it is registered last, so every earlier narrower overload stays reachable and mypy reports no overload-cannot-match. A bind_exp_scalar helper shares the registration across the four signatures. Two `b` defaults are supplied through py::arg_v with an explicit repr so the stub stays valid against the widened 3.0.4 annotations, rather than letting pybind11 format the default itself: - The numpy_scalar overloads use a `numpy.<type>(...)` repr. Otherwise pybind11 formats the default from the numpy scalar's own repr -- `np.<type>(...)` under numpy 2.x -- an unimported `np` alias that mypy rejects; naming the imported `numpy` module lets stubgen evaluate it and emit an elided `...`. - The cytnx_complex128 overload uses a literal `...` repr. pybind11 3.0.4 annotates the parameter `SupportsComplex | SupportsFloat | SupportsIndex`, which does not include the builtin `complex` (typeshed's `complex` has no `__complex__`), so a `0j` default would be rejected as an incompatible default. A `numpy.complex128(...)` repr would also elide, but it would misdescribe the default since this overload accepts a Python `complex`, not a numpy scalar, so `...` is used to state "elided" without claiming a type. The defaults still exist at runtime, so mypy.stubtest is unaffected, and tools/generate_stubs.py needs no special-casing for these arguments. numpy complex64 scalars are routed through the complex128 kernel because cytnx::linalg::Exp returns incorrect values for complex64 tensors (#914), on which both ExpH and ExpM depend; drop that reroute once that is fixed. Add a regression test that ExpH and ExpM accept Python and numpy scalars of every supported dtype, individually and in mixed (a, b) pairs, and match a NumPy eigendecomposition reference, with a dedicated complex64-matches-complex128 case. Co-Authored-By: Claude <noreply@anthropic.com>
After rebasing onto current master (Axpy API removal #787f9d8c, rank-zero support #1026, Exp/Expf consolidation #1047), the committed stubs no longer matched what pybind11-stubgen 2.5.5 produces from the current bindings. The textual rebase auto-merged the .pyi files but generated files must be regenerated, not merged. Regenerated all stubs from a fresh openblas-cpu build (pybind11 3.0.4, pybind11-stubgen 2.5.5). Net effect over the rebased tree is Device.pyi and __init__.pyi only (the drift from #1026/#1047); linalg.pyi already matched. Regeneration is idempotent, and `mypy.stubtest cytnx.cytnx.linalg` reports no ExpH/ExpM overload-cannot-match. linalg_exp_test.py: 26 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
262d3d3 to
82824e8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82824e8134
ℹ️ 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".
…-errors Co-Authored-By: Claude <noreply@anthropic.com>
Regenerated with Python 3.10 (the requires-python floor) per CONTRIBUTING.md's stub regeneration recipe, after merging origin/master into this branch. Two kinds of drift surface: - The pybind11-stubgen run that produced the previous regeneration used a newer interpreter than the 3.10 floor, so `_from_numpy`'s parameter was annotated `collections.abc.Buffer` (added to collections.abc only in Python 3.12) instead of the 3.10-compatible `typing_extensions.Buffer`. Regenerating with 3.10 restores the compatible annotation and the `import typing_extensions` it depends on. - The merged-in master history includes commits that changed pybind bindings without regenerating the committed stubs in the same commit (e.g. "refactor(pybind): bind in-place methods directly, drop the c__* shadow API (#779)", which removed Bond's clear_type/set_type/ c_getDegeneracy_refarg/c_group_duplicates_refarg/c_redirect_ bindings and added UniTensor.combineBond/norm/set_label_/set_name_/tag_ and a Tensor.norm binding). This regeneration catches the committed stubs up to those bindings. `python -m mypy.stubtest cytnx.cytnx` reports zero errors against linalg.pyi; the remaining errors are entirely in __init__.pyi's Tensor/UniTensor operator overloads, the pre-existing baseline this PR's description already scopes out to a dedicated follow-up (#928). Co-Authored-By: Claude <noreply@anthropic.com>
da83f1f to
bbdd1c8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbdd1c8a62
ℹ️ 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".
pybind11 3.0.4 renders a cytnx_complex128 parameter as typing.SupportsComplex | typing.SupportsFloat | typing.SupportsIndex. typeshed's builtin complex has no __complex__ (real-valued dunders like __float__ don't satisfy SupportsComplex, which requires __complex__ specifically), so SupportsComplex alone rejects a plain Python complex literal under strict type checkers even though the binding accepts it directly (e.g. cytnx.linalg.ExpH(t, 2+1j)). Add a sanitize() substitution that prefixes the union with `complex |` so the emitted annotation covers the full accepted range. Co-Authored-By: Claude <noreply@anthropic.com>
Reflects the complex | prefix added to every SupportsComplex | SupportsFloat | SupportsIndex union in the committed stubs, including ExpH/ExpM's a/b parameters (cytnx.linalg.ExpH(t, 2+1j) and the mixed ExpH(t, 2.0, 1+0j) case now type-check). Co-Authored-By: Claude <noreply@anthropic.com>
Move the fix for ExpH/ExpM's stub-visible complex parameter type from tools/generate_stubs.py's post-processing regex to the binding itself, per review feedback that the generator script shouldn't need to know about this. pybind11's own type_caster<std::complex<T>> (pybind11/complex.h) hardcodes the PARAMETER annotation as "typing.SupportsComplex | typing.SupportsFloat | typing.SupportsIndex", omitting builtin `complex` -- typeshed's `complex` only gained `__complex__` in Python >= 3.11, so under this project's Python 3.10 floor a stub generated from that caster rejects a plain complex literal (e.g. `ExpH(t, 2+1j)`) even though the caster's own load() accepts it directly via PyComplex_AsCComplex. The RETURN annotation is already correctly "complex"; only the parameter side is missing it. This is baked into pybind11's own header, not something an ordinary .def() call in this codebase's binding code controls. Added pybind/complex_arg.hpp: a thin ComplexArg wrapper around cytnx_complex128 with its own pybind11::detail::type_caster specialization that delegates load()/cast() entirely to pybind11's own complex caster (no behavior change) and declares the corrected parameter annotation. bind_exp_scalar's cytnx_complex128 overload in linalg_py.cpp now takes ComplexArg for `a`/`b` instead of raw cytnx_complex128. Because the corrected annotation includes `complex`, the `b` default can now use a real `0j` repr instead of the previous elided `...` (pybind11 3.0.4's uncorrected annotation rejected a literal `0j` default as incompatible). Scoped to ExpH/ExpM only, the functions this PR's review comment concerns -- not a global override of pybind11's own type_caster<std::complex<T>>, which would affect every complex128/ complex64-typed parameter across the whole codebase. The generator-side regex in tools/generate_stubs.py stays in place for every other occurrence of this same annotation gap (Scalar, Storage, Tensor, UniTensor, ...) until they migrate to ComplexArg too, tracked separately; the regex's negative lookbehind is idempotent against text this wrapper already produces correctly. Verified: mypy.stubtest reports nothing for ExpH/ExpM specifically (676 total errors, unchanged from before this commit -- all pre-existing, unrelated __init__.pyi issues); pytest pytests/linalg_exp_test.py (26 tests) passes; a direct runtime check (ExpH(t, 2+1j)) confirms behavior is unchanged. 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>
|
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 and the following branch (#1053). Please take over this PR and the following PRs. I am stopping my AI bot tracing on these PRs. |
|
I will merge this PR and close it |
…-errors # Conflicts: # cytnx/cytnx/__init__.pyi
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>
…aining Fix remaining overload-cannot-match errors with the #915 keep-set strategy
3 participants
Context
Follows #912 (now merged), which shipped the committed PEP 561 stubs generated against the pinned pybind11 3.0.4; this PR is based directly on
masterincluding it. Works toward a cleanmypy.stubtestrun againstcytnx.cytnx. stubtest's dominant complaint is theoverload-cannot-matcherrors: many scalar-accepting functions are bound once per C++ scalar dtype, and pybind11-stubgen renders several distinct C++ types onto the same Python type, producing duplicate@typing.overloadsignatures. This PR is the first increment (ExpH/ExpM); it does not make stubtest fully green yet (see "Remaining").What this PR does
Bind
linalg.ExpH/ExpMwith one overload per Python-distinguishable dtype. Each function was bound ~20× per C++ scalar dtype, and stubgen collapses several of those onto the same Python type, so the stub carried duplicate overloads. Keep one binding per Python-distinguishable dtype and drop the dead/redundant ones:float/complexalready binds to thecytnx_double/cytnx_complex128overloads, andnp.float64/np.complex128are builtin subclasses caught by those same overloads, socytnx_float,cytnx_complex64,numpy_scalar<float>andnumpy_scalar<complex64>were unreachable duplicates.int, so replace them with a singlepy::int_overload that dispatches on magnitude to theint64/uint64kernel — one signature covering the full arbitrary-precision Python-int range that no single fixed-width C++ overload can.Under pybind11 3.0.4 the kept overloads render as the
numpy.*scalar types,int(py::int_),typing.SupportsFloat | typing.SupportsIndex(cytnx_double), andtyping.SupportsComplex | typing.SupportsFloat | typing.SupportsIndex(cytnx_complex128). The complex128 annotation is a supertype of the narrower ones, but it is registered last, so each earlier overload stays reachable andlinalg.pyiis mypy-clean forExpH/ExpM(nooverload-cannot-match).Keep the scalar defaults valid at the binding site, not via a stub rewrite. Each
bdefault is passed throughpy::arg_vwith an explicit repr that stubgen renders as an elided..., valid against any annotation, for two reasons:numpy.<type>(...)repr — otherwise pybind11 formats the default from numpy's own repr (np.<type>(...)under numpy 2.x), an unimportednpalias that mypy rejects;cytnx_complex128overload uses a literal...repr — pybind11 3.0.4'sSupportsComplex | SupportsFloat | SupportsIndexannotation excludes the builtincomplex(typeshed'scomplexhas no__complex__), so a literal0jdefault would be flagged as an incompatible default, and anumpy.complex128(...)repr would misdescribe an overload that accepts a plain Pythoncomplex.The defaults still exist at runtime, so stubtest is unaffected and
tools/generate_stubs.pyneeds no special-casing.Numerical note / issue #914
linalg.Expreturns incorrect values for complex64 (ComplexFloat) tensors, which bothExpHandExpMdepend on. To preserve behaviour, numpy complex64 scalars are routed through the complex128 kernel; this is documented in code and tracked in #914, and the reroute can be removed once the kernel is fixed.Remaining (follow-up PR)
After this PR
linalg.pyiis mypy-clean, but stubtest still cannot run a clean comparison: ~5599overload-cannot-matcherrors remain, now all in__init__.pyi— the Tensor/UniTensor operators (__add__,__mul__,__eq__, in-place/reverse variants,__setitem__, …), each bound ~24× per dtype, plus the generated__hash__/__eq__markers. They need the same binding-site treatment this PR applies toExpH/ExpM; cleaning them up is the bulk of the work and a dedicated follow-up (tracked in #928), after which CI can run stubtest.Testing
pytest pytests/linalg_exp_test.py: 26 passed —ExpH/ExpMaccept every supported Python and numpy scalar dtype individually and in mixed(a, b)pairs, matched against a NumPy eigendecomposition reference, including the complex64→complex128 case.linalg.pyiis mypy-clean:python -m mypy.stubtest cytnx.cytnxreports zero errors for it, and regenerating the stubs is idempotent (thearg_vdefaults reproduce the committed.pyibyte-for-byte).ExpH/ExpMkernels are untouched.🤖 Generated with Claude Code