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

Fix serialization of complex256 objects to JSON #91

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Mar 11, 2024

  1. https://h5web.panosc.eu/h5grove?file=sample.h5
  2. Select /complex256_scalar or /complex256_2D
  3. 500 error

h5grove fails in encoders.py, when serializing complex256 values to JSON with orjson. There's an infinite recursion call in orjson_default, on line 27.

With complex64 or complex128 scalars, I trace two calls to orjson_default:

  1. During the first call, the condition that matches first is isinstance(o, (np.generic, np.ndarray)) => o.tolist() is called and returned.
  2. This triggers a second call, during which the condition that matches first is now isinstance(o, complex) => [o.real, o.imag] is returned.

With complex256, the first call is the same, but on the second call, the condition that matches is again isinstance(o, (np.generic, np.ndarray)) so we recurse infinitely.

If I move the complex condition first, then o.tolist() is never called for complex64 and complex128 numbers (i.e. there's only one call to orjson_default). I can then change the condition to isinstance(o, numbers.Complex) to also match complex256 and the infinite recursion issue is gone.

@@ -23,10 +24,11 @@ def orjson_default(o: Any) -> Union[list, float, str, None]:
if isinstance(o, np.number) and o.dtype.kind == "f" and o.itemsize > 8:
# Force conversion of float >64bits to native float even if it means losing precision
return float(o)
if isinstance(o, numbers.Complex):
# Force conversion of float >64bits to native float even if it means losing precision
return [float(o.real), float(o.imag)]
Copy link
Contributor Author

@axelboc axelboc Mar 11, 2024

Choose a reason for hiding this comment

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

Opening as draft as this casting to float64 doesn't seem to work with complex256 values. When I open /complex256_scalar, I still get [0.0,null] instead of [0.0,inf] (like with /float128_2D). I could not reproduce the issue in a Python REPL, so something weird is going on.

Copy link
Contributor Author

@axelboc axelboc Mar 12, 2024

Choose a reason for hiding this comment

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

/float128_2D is fetched as binary, so inf is kept, whereas /complex256_scalar is fetched as JSON, so inf is converted to null. So all is well. This relates more to silx-kit/h5web#641 (comment)

Though now I'm wondering whether casting to float64 with float() is needed at all... With np.complex256(np.finfo(np.complex256).smallest_normal + np.finfo(np.complex256).max * 1j), I get [0.0, null] both with and without the casting. For complex64, it actually adds "fake" precision, I think, which doesn't seem great.

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 removed the casts to float64.

Copy link
Member

Choose a reason for hiding this comment

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

orjson_default is called recursively so when it is called for a complex and converts it to a list, it is then called again to convert the real and imaginary part with:

if isinstance(o, np.number) and o.dtype.kind == "f" and o.itemsize > 8:
# Force conversion of float >64bits to native float even if it means losing precision
return float(o)

@axelboc axelboc marked this pull request as ready for review March 12, 2024 08:41
@axelboc axelboc requested a review from t20100 March 12, 2024 08:42
@axelboc axelboc merged commit 62b17c7 into main Mar 13, 2024
1 check passed
@axelboc axelboc deleted the complex256 branch March 13, 2024 08:54
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