Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type caster for complex numbers #292

Merged
merged 7 commits into from
Sep 29, 2023

Conversation

gillesdegottex
Copy link
Contributor

@gillesdegottex gillesdegottex commented Sep 5, 2023

This is an attempt to add support for complex numbers using a type_caster (previously discussed as #286 ).

With test code:

#include <nanobind/nanobind.h>
#include <nanobind/stl/vector.h>
#include <nanobind/stl/complex.h>

namespace nb = nanobind;

#include <iostream>

typedef double value_t;
// typedef float value_t;

std::complex<value_t> foo(const std::complex<value_t>& x){
    std::cout << "x=" << x << " sizeof(complex value)=" << 8*sizeof(x) << "b" << std::endl;
    return x;
}

std::vector<std::complex<value_t>> foo_array(const std::vector<std::complex<value_t>>& x){
    std::cout << "x[0]=" << x[0] << " sizeof(complex element)=" << 8*sizeof(x[0]) << "b" << std::endl;
    std::cout << "x[1]=" << x[1] << " sizeof(complex element)=" << 8*sizeof(x[1]) << "b" << std::endl;
    return x;
}

NB_MODULE(pybar, m) {
    m.def("foo", &foo);
    m.def("foo_array", &foo_array);
}

This works with test commands:

>> python3 -c 'import pybar; import numpy as np; x = 2.7-3.2j; print(pybar.foo(x))'
>> python3 -c 'import pybar; x = [2.7-3.2j, -1j]; print(pybar.foo_array(x))'
>> python3 -c 'import pybar; import numpy as np; x = np.array([2.7-3.2j, -1j]); print(pybar.foo_array(x))'
>> python3 -c 'import pybar; import numpy as np; x = np.array([2.7-3.2j, -1j],dtype=np.complex128); print(pybar.foo_array(x))'

However, it fails when numpy input is 32b float (i.e. np.complex64), ex:

>> python3 -c 'import pybar; import numpy as np; x = np.array([2.7-3.2j, -1j],dtype=np.complex64); print(pybar.foo_array(x))'
<string>:1: ComplexWarning: Casting complex values to real discards the imaginary part
[...]

This happens, no matter if I'm using std::complex<float> or std::complex<double> in test code.
My understanding is that a PyComplex seems to be built at some point assuming double size (because base python (not numpy) complex type is always double) and I can't find where this is built (I guess before call to type_caster<std::complex<T1>>::from_python(.) ).
If anybody has an idea about this issue, it is very much welcome!

Even though forcing to 64b float could be enough, this is clearly not optimal both in terms of efficiency and memory.

Elements to review:

  • See copyright notice. I don't know how you handle this in nanobind.
  • Tests: Done.
  • np.complex64 inputs, as mentioned above: Now supported.
  • Unnecessary use of make_caster<T1> I think. See TODO tag in file stl/complex.h: Cleaned.

Copy link
Owner

@wjakob wjakob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add test coverage to the official test suite? (this should validate the casting in both directions for std::complex<float> and std:::complex<double>).

include/nanobind/stl/complex.h Outdated Show resolved Hide resolved
include/nanobind/stl/complex.h Outdated Show resolved Hide resolved
include/nanobind/stl/complex.h Outdated Show resolved Hide resolved
@gillesdegottex
Copy link
Contributor Author

I'll see for coding the tests, it might take me a bit of time.
Should I get inspired by a specific test file? (otherwise tests/test_ndarray.* seems a descent starting point to me)
This would go in tests/test_stl_complex.*, correct?

@wjakob
Copy link
Owner

wjakob commented Sep 6, 2023

You can put tests into the existing STL type casters (it is part of that). I don't think it needs to be that complicated: show that you can pass a complex number to C++ and back, for single and double versions.

@wjakob
Copy link
Owner

wjakob commented Sep 20, 2023

I'm checking in on the status of this PR -- will you be able to add test coverage @gillesdegottex ?

@gillesdegottex
Copy link
Contributor Author

Give me up to next Monday plz, and just close it if I can't do it by then.

@gillesdegottex
Copy link
Contributor Author

gillesdegottex commented Sep 24, 2023

All done.
Side bonus: complex64 is also handled now. I suspect it could be faster though (see TODO).

The 9 failing tests do so because of numpy module not being installed on the corresponding docker image.
I can add a condition to run only the numpy-related tests if the module is present.
I think it would be better to install the package, since complex numbers are very likely to be used when numpy is also used.

@wjakob wjakob merged commit 90946c3 into wjakob:master Sep 29, 2023
12 of 21 checks passed
wjakob pushed a commit that referenced this pull request Sep 29, 2023
@wjakob
Copy link
Owner

wjakob commented Sep 29, 2023

I made a number of changes to this PR and merged it as commit dcbed4f.

@gillesdegottex
Copy link
Contributor Author

Great thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants