Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Conformant ZarrV3 codecs and fill values #193
Conformant ZarrV3 codecs and fill values #193
Changes from 2 commits
6b7abe2
bca0aab
384ff6b
4c5f9bd
1dd3370
d92c75c
0ed8362
f4485fa
3cc1254
0123df7
332bcaa
c51e615
0083f77
3c00071
207c4b5
c573800
ef0d7a8
588e06b
725333e
72df108
d1e85cb
1e2b343
7f1c189
908e332
0df332d
ca6b236
b7426c5
dac21dd
e968772
9a98e57
7590b87
e9fbc8a
14bd709
acdf0d7
4ba323a
e14e53b
b808ded
b052f8c
01a3980
c37d9e5
17b30d4
f6b596a
eb6e24d
c85bd16
ca435da
3017951
ccf0b73
32ba135
64f446c
1c590bb
9797346
c833e19
701bcfa
08c988e
e6076bd
d684a84
4cb4bac
d352104
3d89ea4
c9dd0d9
db5b421
9a1da32
9b2b0f8
9ef1362
eb16bc1
5f1b7f9
519d45d
76e9c8e
000c520
25d04b9
c2e7279
145960a
c051f04
7a65fbd
7b09324
2c59256
50c3dcd
ab97e63
0be0728
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It seems like the default fill value for float is 0.0:
(I'm not sure where on the Array / Store / Other object that information lives.)
It'd be nice if zarr-python had this as a constant that we could reuse. Would that make sense, or is there some reason not to I'm missing?
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 also see a reference to NaN as a default at https://github.com/zarr-developers/VirtualiZarr/pull/193/files#diff-f5a7b84b3378d903e91ebf06f2db06dca5ad55d12e7c3bf8537a9b9bb1c4cfa0R361 (present on main).
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 think the specification doesn't require a specific number, just that it not be null. See the note at the bottom of the
fill_value
section https://zarr-specs.readthedocs.io/en/latest/v3/core/v3.0.html#fill-value . Howeverzarr-python
does default to 0 eventually and not NaN https://github.com/zarr-developers/zarr-python/blob/37a8441c20dae3b284803bb1b0d2e6c8f040fb3e/src/zarr/array.py#L231C9-L235C31 . I may have had some trouble with the unit tests, but I think it's better to be as similar as possible tozarr-python
, I'll change the defaults to 0s.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 think this could also be deferred to a later PR, especially if the true solution is to make it clearer what the default is upstream.