refactor(pybind)!: remove the cytnx.Scalar Python binding (#1045) - #1104
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 removes the cytnx.Scalar class from the Python bindings, replacing its usage with native Python and NumPy scalar types (such as cytnx_double and cytnx_complex128) across Tensor, UniTensor, and linear algebra functions like Gemm, Gemm_, Ger, and Lanczos_Exp. Feedback was provided to release the GIL in the newly added Ger overloads to maintain consistency with other computationally intensive linear algebra operations.
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.
| "Ger", [](const Tensor &x, const Tensor &y) { return cytnx::linalg::Ger(x, y); }, py::arg("x"), | ||
| py::arg("y")); |
There was a problem hiding this comment.
For consistency with other linear algebra operations that can be computationally expensive, consider releasing the GIL here. Ger performs an outer product, which can be intensive for large tensors. This should be applied to all three Ger overloads.
| "Ger", [](const Tensor &x, const Tensor &y) { return cytnx::linalg::Ger(x, y); }, py::arg("x"), | |
| py::arg("y")); | |
| "Ger", [](const Tensor &x, const Tensor &y) { return cytnx::linalg::Ger(x, y); }, py::arg("x"), | |
| py::arg("y"), py::call_guard<py::gil_scoped_release>()); |
There was a problem hiding this comment.
For context: Ger's binding didn't release the GIL on master either, so this isn't introduced by the Scalar removal — I kept the existing behavior to keep the PR scoped. Ger is pure C++ compute (no Python callbacks), so adding py::call_guard<py::gil_scoped_release>() to both Ger overloads is safe; I'd prefer to land it as a small separate consistency change (a couple of other linalg bindings are in the same boat) rather than widen this PR. Gemm/Gemm_/Lanczos_Exp already release it.
![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: 52f15161da
ℹ️ 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".
| m_linalg.def( | ||
| "Ger", | ||
| [](const Tensor &x, const Tensor &y, cytnx_double a) { return cytnx::linalg::Ger(x, y, a); }, |
There was a problem hiding this comment.
For Python callers replacing cytnx.Scalar(np.float32(...)) or cytnx.Scalar(np.complex64(...)) with the underlying numpy scalar, these new coefficient overloads only create Double/ComplexDouble Scalars. Ger and the Gemm variants include a/b in dtype promotion (linalg::Ger, linalg::Gemm_), so Float/ComplexFloat inputs now get promoted to Double/ComplexDouble instead of preserving the narrow coefficient dtype; that changes observable result dtypes and can double memory/work. Please add float/complex64/numpy-scalar coefficient overloads rather than forcing the widest types.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6288186, together with the integer case. The coefficients now go through a single runtime ScalarArg dispatcher (pybind/scalar_arg.hpp) that keeps the caller's numpy width: np.float32 → Float, np.complex64 → ComplexFloat, and np.int8/np.uint8 → Int16/Uint16 (the widths pybind's numpy_scalar<T> set doesn't register, #1065). One overload → one stub signature, per @IvanaGyro's #1066.
One correction to the premise: on origin/master a bare np.float32/np.complex64 coefficient actually raised TypeError here (the accept-Scalar overload's implicit conversions didn't accept them), rather than preserving Float32/Complex64 — so this is now accepted and preserved, not a regression that existed before.
#1045 review) Addresses @IvanaGyro's review of #1104: the complex overloads of Gemm/Ger/Gemm_/Lanczos_Exp took a raw cytnx_complex128, so pybind11-stubgen annotated the parameter as 'SupportsComplex | SupportsFloat | SupportsIndex', which omits builtin 'complex' under the project's Python 3.10 floor (typeshed's 'complex' only gained __complex__ in 3.11). A stub generated from that caster rejects a plain complex literal (e.g. Gemm(2+1j, x, y)) at type-check time even though the runtime caster accepts it. Wrap those parameters in complex_arg.hpp's ComplexArg -- it delegates load()/cast() to the cytnx_complex128 caster and only overrides the declared stub annotation to include 'complex' -- matching the ExpH/ExpM bindings already in this file. Testing: openblas-cpu pycytnx build; verified Gemm/Ger/Gemm_/Lanczos_Exp accept plain Python complex literals at runtime with the real overloads unchanged; regenerated linalg.pyi (the four complex annotations now include 'complex'). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed @IvanaGyro's review in 5893f30: the complex overloads of Heads-up: this branch is ~117 commits behind |
Problem: cytnx.Scalar leaked C++'s Scalar value type onto the Python surface. Python code should work with native numbers and derive dtype from a tensor/storage's dtype(), never handle a Scalar object. Fix: - Drop the Scalar binding entirely (delete pybind/scalar_py.cpp, its scalar_binding() decl/call in cytnx.cpp, and its CMakeLists entry). - Remove the redundant accept-Scalar arithmetic/compare overloads from Tensor (tensor_py.cpp) and UniTensor (unitensor_py.cpp). Each has a native double/complex128 sibling plus the numpy_scalar/py::int_ keep set, so plain Python numbers still dispatch unchanged -- including scalar UniTensor '//' and '%', kept per #1049. - Migrate the linalg coefficients that were Scalar (Ger, Gemm, Gemm_, Lanczos_Exp) to native real overloads plus a complex_arg.hpp ComplexArg fallback, matching the existing ExpH/ExpM pattern so a real argument stays real and a Python complex is accepted on 3.10. - Delete the now-obsolete Python Scalar tests (pytests/scalar_variant_test.py and the Scalar-constructor cases in keep_set_dispatch_test.py); the C++ variant behaviour stays covered by tests/Scalar_test.cpp. Update the __ne__/reverse-op tests to use native operands. - Regenerate the committed .pyi stubs (Scalar class removed; Ger/Gemm/ Gemm_/Lanczos_Exp gain real/complex overloads). Testing: pytest pytests/ -> 260 passed, 1 skipped. Smoke-checked that cytnx.Scalar is gone, Tensor/UniTensor scalar arithmetic (incl. ut//2.0), and Ger/Gemm/Gemm_/Lanczos_Exp accept native ints/floats/complex with the expected result dtypes. clang-format v14 clean. Closes #1045. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
5893f30 to
2e96b24
Compare
|
Rebased onto current What changed vs. the previous version of this PR, because
Testing on this box (openblas-cpu): |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e96b24867
ℹ️ 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_double a, const Tensor &x, const Tensor &y, cytnx_double b, Tensor &c) { | ||
| cytnx::linalg::Gemm_(a, x, y, b, c); |
There was a problem hiding this comment.
When callers replace cytnx.Scalar(5) with the native 5, this new coefficient overload routes the value through a Double Scalar instead of the previous int64/uint64 Scalar path. Gemm_/Gemm/Ger include these coefficients in Type.type_promote before running the kernels, so float tensors now get promoted to Double for integer coefficients such as Gemm_(5, x_float, y_float, 0, c_float); add a py::int_ dispatch overload like the tensor keep-sets use so native Python integers keep their integer Scalar dtype instead of widening to Double.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified and fixed in 6288186. You're right — reproduced against a from-scratch build of origin/master: Gemm(5, float, float) was Float, and my earlier cytnx_double overload made it Double (on master an integer coefficient resolved to an int64/uint64 Scalar, which doesn't upcast a Float result).
Rather than a py::int_ overload, the coefficients now go through a single ScalarArg dispatcher (pybind/scalar_arg.hpp) that infers the coefficient's cytnx dtype from the caller's actual Python/numpy type at runtime — the single-dispatcher model @IvanaGyro proposes in #1066, not a per-dtype overload ladder. Result, all matching master again: Gemm(5,f,f) / Gemm(np.int64(5),…) / Gemm(np.int32(5),…) / Ger(f,f,5) → Float; 2.0 → Double; 2+1j → ComplexDouble. It also closes the np.int8/np.uint8 gap (#1065) and now preserves narrow numpy floats/complex that the old accept-Scalar overload rejected outright.
There was a problem hiding this comment.
#1066 is for list args only. There should be more efficient way for scalar args.
There was a problem hiding this comment.
Thanks for the correction — I over-applied #1066 (which, as you say, is about list args and their mixed-element / empty-list problems) to scalar args, where those problems don't exist. Reverted to the minimal double + ComplexArg binding in 2e96b248.
The only thing left is Codex's note that a bare integer coefficient now widens a Float result to Double (correctness-neutral, and only for the rare Float-tensor + integer-coefficient case). If you do want to preserve the coefficient's dtype, I'll do it with explicit numpy_scalar<T> overloads (the bind_exp_scalar pattern) as a separate, consistent change rather than a dispatcher — just let me know if it's worth it.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1104 +/- ##
==========================================
- Coverage 73.25% 72.97% -0.28%
==========================================
Files 225 225
Lines 27802 27692 -110
Branches 71 71
==========================================
- Hits 20365 20209 -156
- Misses 7416 7462 +46
Partials 21 21
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
| // Ger's scaling coefficient `a` was formerly a cytnx.Scalar; the Python | ||
| // surface no longer exposes Scalar, so split it into native real/complex | ||
| // overloads plus a no-`a` form. The no-`a` form calls the C++ default | ||
| // (Scalar()), whose Void dtype keeps `a` out of the output-dtype promotion | ||
| // (see src/linalg/Ger.cpp) -- passing 1.0 explicitly would not reproduce it. | ||
| m_linalg.def( | ||
| "Ger", [](const Tensor &x, const Tensor &y) { return cytnx::linalg::Ger(x, y); }, py::arg("x"), | ||
| py::arg("y")); |
There was a problem hiding this comment.
BTW, this is not really the BLAS ger.
There was a problem hiding this comment.
Agreed — the binding just forwards to cytnx::linalg::Ger; the name predates this PR and its semantics are unchanged here. Happy to track a rename separately if it's a misnomer worth fixing.
|
Pushed Codex correctly caught that binding the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62881865c6
ℹ️ 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("__add__", | ||
| [](cytnx::Tensor &self, const cytnx::cytnx_double &rhs) { return self.Add(rhs); }) |
There was a problem hiding this comment.
When callers migrate cytnx.Scalar(np.int8(...))/np.uint8(...) to the underlying numpy scalar, this keep-set has no 8-bit numpy overload and py::int_ does not accept numpy integer scalars, so those operands fall through to the double fallback here. That changes cases like cytnx.zeros([1], dtype=Type.Float) + np.int8(1) from the old integer-Scalar promotion path to a Double result; please add explicit int8/uint8 handling, or route these scalar operands through the same dtype-inspecting ScalarArg used for linalg.
Useful? React with 👍 / 👎.
| // Build a cytnx::Scalar from an arbitrary Python number (Python | ||
| // bool/int/float/complex, or any numpy scalar), inferring the dtype at | ||
| // runtime. Returns false for anything that is not a number. | ||
| inline bool load_scalar_arg(pybind11::handle src, cytnx::Scalar &out) { |
There was a problem hiding this comment.
I feel that this dispatcher is slow. Will explicitly overload for each numpy type be faster?
There was a problem hiding this comment.
You're right — reverted. I've dropped scalar_arg.hpp and put the branch back to 2e96b248 (the commit you approved), so Ger/Gemm/Gemm_/Lanczos_Exp are the simple double + ComplexArg binding again. Agreed the runtime numpy reflection isn't the right tool for a single scalar coefficient.
There was a problem hiding this comment.
You're right — reverted. I've dropped
scalar_arg.hppand put the branch back to2e96b248(the commit you approved), soGer/Gemm/Gemm_/Lanczos_Expare the simpledouble+ComplexArgbinding again. Agreed the runtime numpy reflection isn't the right tool for a single scalar coefficient.
Sorry. It seems that I was wrong.
There was a problem hiding this comment.
I investigated pybind11 3.0.4's scalar casters and overload resolution, and benchmarked alternatives with the same shape as Ger(x, y, a) (two bound arguments followed by the scalar).
Conclusion: the concern about the original dispatcher is valid, but explicit external pybind11 overloads for every NumPy scalar type are not generally faster. The dispatcher can be made substantially cheaper without an overload ladder. More importantly, the current reverted head (2e96b248) has a correctness problem and should not be merged unchanged.
Correctness problem in the current double + ComplexArg bindings
The current binding registers the double overload before ComplexArg for Ger, Gemm, Gemm_, and Lanczos_Exp. For np.complex64(3 + 4j):
- Neither overload matches during pybind11's no-conversion pass.
- During the conversion pass, the earlier
doubleoverload invokesPyFloat_AsDouble. - NumPy converts the value to
3.0, emitsComplexWarning, and discards4j. - The later
ComplexArgoverload is never tried.
I reproduced this with pybind11 3.0.4. It is especially risky for Lanczos_Exp, where a complex time step can silently become real. The same binding also maps np.float32/integer coefficients to Double and np.complex64 to ComplexDouble; because Ger/Gemm include coefficients in dtype promotion, that can change the result dtype and select double-precision storage/kernels.
This behavior follows from pybind11's floating-point caster, complex caster, and registration-order-based overload resolution.
Why a per-dtype overload ladder is not the efficient solution
Each py::numpy_scalar<T> caster is efficient by itself: it checks a cached NumPy scalar type and extracts with PyArray_ScalarAsCtype (implementation). The cost comes from pybind11's outer overload dispatcher:
- overloads are attempted sequentially in registration order, potentially in both no-conversion and conversion passes;
- for
Ger(x, y, a), every failed scalar overload also reloads the preceding Tensor arguments; - later scalar types become progressively slower;
Gemm_has two independently typed coefficients, so complete explicit coverage becomes combinatorial.
A representative pybind11 3.0.4 microbenchmark gave:
| Strategy | Approximate total call time |
|---|---|
| Baseline bound call | 0.10 µs |
Original dtype.kind/itemsize reflection |
0.12 µs for Python numbers; 0.40–0.46 µs for NumPy scalars |
One caster backed by exact std::variant alternatives |
0.15–0.31 µs |
| One caster using cached NumPy type-pointer dispatch | 0.11–0.15 µs |
| External overload ladder | 0.21 µs for the first type; 0.8–2.2 µs for later types |
The absolute values are machine-dependent, but the ordering was stable: an overload ladder only favors whichever type is registered first. The original reflection overhead is only a few tenths of a microsecond and is normally negligible relative to allocating and computing a GER/GEMM result; incorrect promotion to a wider kernel is likely more expensive.
Recommended implementation
Restore a single input-only ScalarArg, but remove Python-level dtype.kind/itemsize reflection. Implement its load() using either:
- a
std::variantof exactpy::numpy_scalar<T>casters followed bypy::bool_,py::int_,double, andComplexArg(simpler and already fast); or - cached exact NumPy scalar type-pointer checks with direct extraction (fastest in the microbenchmark).
The caster should immediately convert the selected value to cytnx::Scalar while the GIL is held; the computational lambda then receives only C++ data, so the existing GIL-release guards remain safe. Put exact NumPy alternatives before broad Python conversions, with Boolean before integer.
This keeps one binding and one union stub signature, preserves np.float32/np.complex64, handles np.int8/np.uint8 by widening them to Cytnx Int16/Uint16, avoids repeated Tensor conversions, and handles the two Gemm_ coefficients independently.
So my recommendation is:
- Do fix the correctness/dtype issue; do not optimize it with many pybind11 overloads.
- Use one exact-type
ScalarArgcaster, preferably variant-backed unless profiling justifies cached-pointer code. - Add regression tests for nonzero-imaginary
np.complex64, narrow result dtypes, native integers, and mixedGemm_coefficient types. - Treat releasing the GIL for
Geras a separate, more meaningful performance improvement.
— GPT-5.6 Sol Max
There was a problem hiding this comment.
A correction to my earlier recommendation: yes, for this PR I would prefer the cached NumPy type-pointer caster over the variant-backed caster.
I originally favored the variant because it composes pybind11 casters and avoids depending directly on pybind11::detail. That was too conservative here: Cytnx pins pybind11 == 3.0.4 exactly, the cached implementation is small and isolated, and a future pybind11 bump will compile-test it. The variant remains reasonable, but it is not the best-performing choice shown by the benchmark.
This is worth implementing primarily to restore correct coefficient dtype/complex-value behavior; the dispatch-speed improvement is secondary. I compiled and tested the following core against pybind11 3.0.4 + NumPy 2.3.5 for all 13 standard NumPy scalar types, including np.complex64(3+4j):
#include <pybind11/numpy.h>
template <typename NumpyT, typename CytnxT = NumpyT>
bool try_numpy_scalar_exact(pybind11::handle src, cytnx::Scalar &out) {
// pybind11's numpy_scalar<T> caster already owns a lazily cached type pointer.
using caster = pybind11::detail::type_caster<pybind11::numpy_scalar<NumpyT>>;
auto *expected = reinterpret_cast<PyTypeObject *>(caster::target_type().ptr());
if (Py_TYPE(src.ptr()) != expected) return false;
NumpyT value{};
pybind11::detail::npy_api::get().PyArray_ScalarAsCtype_(src.ptr(), &value);
out = cytnx::Scalar(static_cast<CytnxT>(value));
return true;
}
inline bool load_standard_numpy_scalar(pybind11::handle src, cytnx::Scalar &out) {
using namespace cytnx;
// Put the common coefficient types first; each failed check is one pointer comparison.
return try_numpy_scalar_exact<float, cytnx_float>(src, out)
|| try_numpy_scalar_exact<double, cytnx_double>(src, out)
|| try_numpy_scalar_exact<std::complex<float>, cytnx_complex64>(src, out)
|| try_numpy_scalar_exact<std::complex<double>, cytnx_complex128>(src, out)
|| try_numpy_scalar_exact<bool, cytnx_bool>(src, out)
|| try_numpy_scalar_exact<std::int8_t, cytnx_int16>(src, out)
|| try_numpy_scalar_exact<std::uint8_t, cytnx_uint16>(src, out)
|| try_numpy_scalar_exact<std::int16_t, cytnx_int16>(src, out)
|| try_numpy_scalar_exact<std::uint16_t, cytnx_uint16>(src, out)
|| try_numpy_scalar_exact<std::int32_t, cytnx_int32>(src, out)
|| try_numpy_scalar_exact<std::uint32_t, cytnx_uint32>(src, out)
|| try_numpy_scalar_exact<std::int64_t, cytnx_int64>(src, out)
|| try_numpy_scalar_exact<std::uint64_t, cytnx_uint64>(src, out);
}load_scalar_arg() should call this before broad PyFloat_Check/PyComplex_Check branches, because under NumPy 2.x np.float64 and np.complex128 are subclasses of Python float and complex. Keep the existing Python bool/int/float/complex handling for native values.
The exact Py_TYPE comparison is the hot path. If NumPy scalar subclasses must also be accepted, add a rare fallback pass using PyObject_TypeCheck(src.ptr(), expected) after the exact/native paths. That preserves subclass support without slowing standard scalar calls.
The deliberate tradeoff is the two pybind11::detail references. They mirror pybind11 3.0.4's own numpy_scalar<T> caster, require no NumPy build-time headers, and are reasonable here because pybind11 is exactly pinned. This dependency should be noted beside the helper and revalidated whenever the pin changes.
— GPT-5.6 Sol Max
6288186 to
2e96b24
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e96b24867
ℹ️ 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("__add__", | ||
| [](UniTensor &self, const cytnx::cytnx_double &rhs) { return linalg::Add(self, rhs); }) |
There was a problem hiding this comment.
When users replace cytnx.Scalar(np.int8(...))/np.uint8(...) with the underlying numpy scalar for UniTensor arithmetic, this keep-set has no 8-bit numpy overload and py::int_ does not accept numpy integer scalars, so the value falls through to this cytnx_double overload. linalg::Add then promotes Scalar(lc).dtype() with the UniTensor dtype (src/linalg/Add.cpp:360), so cases such as cytnx.UniTensor(cytnx.zeros([1], dtype=Type.Float)) + np.int8(1) now produce a Double UniTensor instead of following the old integer-Scalar path that widened to Int16 and preserved Float output.
Useful? React with 👍 / 👎.
2 participants
Problem
cytnx.Scalaris a C++ type-erasure helper that adds nothing on the Python surface — there is no native float32, and acytnx.Scalarobject is worse ergonomics than a plain Python number whose precision the caller already knows from the tensor'sdtype. Direction confirmed by @yingjerkao in #1045.Fix
pybind/scalar_py.cppand its declaration/registration inpybind/cytnx.cppand the CMake source list.const Scalar&overloads from everyTensor/UniTensorarithmetic/comparison/__setitem__keep-set. Native int/float/complex and numpy scalars are already handled by sibling overloads, and withScalarunregistered nothing in Python can produce acytnx.Scalarto match them — so removal is behaviour-preserving for all native inputs.linalgGer/Gemm/Gemm_/Lanczos_ExpfromScalarcoefficient args to nativedouble/complex128overloads.Gerkeeps a no-aform so the C++Void-default (which keepsaout of output-dtype promotion, seesrc/linalg/Ger.cpp) is preserved exactly.cytnx.Scalar(...); regeneratelinalg.pyiand surgically dropScalarfrom__init__.pyi.Scope note: the rest of
__init__.pyiis independently stale (it predates the operator keep-set collapse, #916/#928, and was never regenerated). A full regeneration produces a ~2600-line diff of unrelated churn, so this PR only removes theScalarentries and leaves the broader stub reconciliation for a dedicated stub-hygiene pass.C++
Scalaris unaffected — this is purely the Python binding surface.Testing
openblas-cpupycytnxbuild clean.pytest pytests/→ 139 passed, 1 skipped.cytnx.Scalaris gone and native int/float/complex/numpy arithmetic (both directions), floordiv, andGer/Gemmreal+complex coefficients still work.Closes #1045. Refs #1000.
🤖 Generated with Claude Code