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 Complex Number Support #233

Merged
merged 4 commits into from
Oct 28, 2024
Merged

Conversation

calgray
Copy link
Contributor

@calgray calgray commented Oct 15, 2024

NPY and Zarr support complex floating point tensors with the <c dtype prefix, but are not yet supported by z5. This changeset adds support for 3 fixed sized complex dtypes made available and tested using the C++ standard library std::complex<T>.

External to this PR I've tested loading complex tensors from Zarr but not N5, if there's evidence N5 does support complex numbers I'd be open to exploring support for N5 too.

@constantinpape
Copy link
Owner

Hi @calgray ,
thanks for proposing this contribution. It looks like std::complex is only available with the C++ standard 23, see https://en.cppreference.com/w/cpp/numeric/complex. In order to support older standards it would be best to only compile the respective code if C++ 23 is supported by the compiler.

@calgray
Copy link
Contributor Author

calgray commented Oct 16, 2024

Hi @constantinpape, std::complex<T> has been in the standard library since C++98 (the layout is compatible too with C99 using complex.h). The changes slated for C++26 is that it no longer requires specialized implementations for each floating point type (possibly opening up support for std::float16_t and std::bfloat16_t).

I've yet build and update the tests locally (looks like #include <complex> is missing from some files), but will do so shortly.

@constantinpape
Copy link
Owner

I see, thanks for the clarification! Then this seems to be just due to missing includes.

Copy link
Owner

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

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

Hi @calgray ,
I only had time to check this now, but it looks good to go from my side now. Anything else you want to add? Otherwise I can merge it.

@calgray
Copy link
Contributor Author

calgray commented Oct 28, 2024

I noticed there is some dtype related code in z5py/dataset.py with tests. I don't use these bindings, but I'm happy to create a small separate MR to add complex support there too as it should be a trivial amount of work to add

Comment on lines 19 to 20
'float32', 'float64'
'float32', 'float64', # 'float128',
'complex64', 'complex128', # 'complex256',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've observed good behaviour with complex64 and complex128 in both Zarr and N5 format, but complex256 is problematic, possibly due to a dependency's limited support for float128. I don't particularly need that precision level, but it is the last precision level supported by npy and could be a good addition in the future once the problem is better understood.

Happy to remove all mentions of float128 and complex256, or leave them as is to raise visibility about missing functionality.

Copy link
Owner

Choose a reason for hiding this comment

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

I think leaving it as a comment is good to point to the possibility of adding it.

@constantinpape
Copy link
Owner

The tests are now broken because it tries to run them for complex256 from python:

  File "/Users/runner/micromamba/envs/z5-build-env/lib/python3.11/site-packages/z5py/dataset.py", line 37, in Dataset
    np.dtype('complex256'): 'complex256'}
    ^^^^^^^^^^^^^^^^^^^^^^
TypeError: data type 'complex256' not understood

Could you fix that by removing complex256 from the tests? Then it's good to go! :)

I noticed there is some dtype related code in z5py/dataset.py with tests. I don't use these bindings, but I'm happy to create a small separate MR to add complex support there too as it should be a trivial amount of work to add

It looks like you took care of this already, or am I missing something?

@calgray
Copy link
Contributor Author

calgray commented Oct 28, 2024

Looks like some of these tests got skipped in my testing due to not installing zarr python package in my test environment..

The main issue seems to be the writer defaulting to fill_value = [0, 0], of which the zarr library can handle for zarr format, but the z5 reader can't handle. I'll try adding support for both, otherwise I'll default it to a scalar or null.

@calgray
Copy link
Contributor Author

calgray commented Oct 28, 2024

The tests are now broken because it tries to run them for complex256 from python:

  File "/Users/runner/micromamba/envs/z5-build-env/lib/python3.11/site-packages/z5py/dataset.py", line 37, in Dataset
    np.dtype('complex256'): 'complex256'}
    ^^^^^^^^^^^^^^^^^^^^^^
TypeError: data type 'complex256' not understood

Could you fix that by removing complex256 from the tests? Then it's good to go! :)

Interesting, that's indicative that the Windows test run is using a numpy version prior to float128 support (I've been testing and checking Ubuntu). I think in this case I'll move the python support to a separate PR.

@constantinpape
Copy link
Owner

Thanks for fixing the tests. I agree it's best to do the python bindings in a separate PR.
Thanks for the contribution!

@constantinpape constantinpape merged commit a6a5073 into constantinpape:master Oct 28, 2024
12 checks passed
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