Skip to content

Conversation

@constantinpape
Copy link

@constantinpape constantinpape commented Jan 23, 2020

Add custom JsonEncoder / JsonDecoder to properly handle nan and inf attributes, see #412.
Also adds a test to ensure these values are properly encoded / decoded.
This is still WIP because:

  • I haven't tested the solution myself yet.
  • Potentially one could simplify the code here and here with this solution, but I am not sure this is a good idea, because I don't know if we can always assume that the metadata passes through zarr.util.json_dumps/json_loads
  • Potentially one could also add json encoding for numpy dtypes; this is an error that always annoys me a lot, but it's unrelated to Special treatment of NaN in JSON user attributes #412.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • AppVeyor and Travis CI passes
  • Test coverage is 100% (Coveralls passes)

@jakirkham
Copy link
Member

Should the encoder/decoder bits be moved to Numcodecs?

@constantinpape
Copy link
Author

Should the encoder/decoder bits be moved to Numcodecs?

Happy to move it there if you think that would make more sense. Would you put it here?

@constantinpape
Copy link
Author

Tests pass, so the solution seems to be viable.

@joshmoore
Copy link
Member

Sorry, @constantinpape. Somehow I missed this when reviewing #933 which maybe should been considered together.

@joshmoore
Copy link
Member

Zarr/JSON encoding came up again yesterday during the community call. I will try to resolve the conflicts here and see how things stand.

@jhamman jhamman added the stale label Dec 7, 2023
@jhamman
Copy link
Member

jhamman commented Dec 7, 2023

This has unfortunately gone stale. Will it be revived or should we close this?

@jhamman
Copy link
Member

jhamman commented Feb 13, 2024

Closing this out as stale. We'll keep the feature request open.

@jhamman jhamman closed this Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants