-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
h5grove/encoders.py
Outdated
@@ -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)] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
Lines 23 to 25 in a248a20
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) |
/complex256_scalar
or/complex256_2D
h5grove fails in
encoders.py
, when serializingcomplex256
values to JSON with orjson. There's an infinite recursion call inorjson_default
, on line 27.With
complex64
orcomplex128
scalars, I trace two calls toorjson_default
:isinstance(o, (np.generic, np.ndarray))
=>o.tolist()
is called and returned.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 againisinstance(o, (np.generic, np.ndarray))
so we recurse infinitely.If I move the complex condition first, then
o.tolist()
is never called forcomplex64
andcomplex128
numbers (i.e. there's only one call toorjson_default
). I can then change the condition toisinstance(o, numbers.Complex)
to also matchcomplex256
and the infinite recursion issue is gone.