Improve type_caster for complex number types.#854
Conversation
|
Like with the floating point type casters, could you move the (now fairly large) body of the converter to the static library component? |
|
Sure. Do you prefer not to include |
|
It's fine to include in the cpp file. |
|
I ran the
The old complex caster used the C API functions from the stable abi regardless of whether The new complex caster uses The new caster uses the same complex C API functions as the old one did when Also, templating is removed from For non-STABLE_ABI builds, the new caster is much faster, and for STABLE_ABI builds it's faster. As this Note mentions, the behavior of a stable abi function can change. In particular, the workaround is no longer needed in version 3.13. But, it is still perfectly correct to define Py_LIMITED_API=0x030C0000 and build with the workaround code in place. |
98def19 to
92d9cb3
Compare
|
Thanks! |
|
Actually, one question I realized after merging: is the |
|
Yes, the code would still be correct without that check, but it would be slower. Referring to the table in my original comment for this PR, the case The old code effectively had this check, because it used The new code already has the boolean |
|
Thank you for clarifying! |
|
We just bumped our project from using Nanobind 2.4.0 to Nanobind 2.7.0. I was surprised to learn that nb::isinstance<std::complex<double>>(python_obj)no longer matches the complex-valued type After looking in the Nanobind changelog and browsing the source code a bit, I seem to have narrowed this change in behaviour down to this PR. I guess it might have something to do with the fact that the type_caster for complex numbers was changed with this PR, although I might be wrong. Was this an intended change in behaviour or an accidental one? Do you intend for |
|
@miklhh Do you include |
|
This PR was primarily intended to avoid unintentional value-changing narrowing conversions (and the resulting loss of numerical precision). Similar was done for real values. See the nice table in #829. I'm not sure what As a wild guess, I see the following: |
|
If I'm understanding the nanobind code correctly, As an alternative to |
|
Thank you both for your answers.
Yes we do.
Should have been more clear. The
We ended up using |
|
@miklhh, @oscargus, There's also https://nanobind.readthedocs.io/en/latest/api_core.html#_CPPv4N8nanobind10isinstanceE6handle6handle Note that The following, I think, will match python's complex or any subclass: |
Results for this PR were obtained with Python 3.12.8 and g++ 14.2.0.
Currently, after importing the module defined in
tests/test_stl.cppastand definingthe following raises a TypeError:
Instead, it is expected that the
__complex__method be called and the assertion be correct.Note that the Stable ABI for PyComplex_RealAsDouble,ImagAsDouble was fixed in version 3.13. See cpython issue 109598
Also, the current complex type_caster does not honor
nb::arg().noconvert().The following test passes
but given
the following tests
DID NOT RAISE <class 'TypeError'>and fail:This PR re-writes the complex type_caster.
Regarding performance, given the extension
and measuring time with
I obtained the following results:
For the last two lines above, note that
I would expect that compiling
new abi3with-DPy_LIMITED_API=0x030D0000would reducecaddf(np.complex64)by 2X, but I don't have Python 3.13 handy.