Repository navigation
feat: extensible write support for accessors - #2658
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/write-compat-profiles #2658 +/- ##
==============================================================
+ Coverage 85.89% 86.04% +0.15%
==============================================================
Files 50 51 +1
Lines 8358 8444 +86
==============================================================
+ Hits 7179 7266 +87
+ Misses 1179 1178 -1
|
ca238c4 to
b4059b2
Compare
7b46343 to
3027d25
Compare
flying-sheep
changed the title
flying-sheep
changed the title
ilan-gold
left a comment
ilan-gold
left a comment
There was a problem hiding this comment.
So I'm not sure I like this design...I'd like to ask in the zarr Zulip about this...
|
It might be worth jamming zarr-developers/zarr-extensions#9 through in advance of this if there is appetite from the zarr maintainers (which there seems to be) |
3027d25 to
4d96389
Compare
|
Can you link to your zarr zulip post please? /edit: found it #Zarr-Python > json in zarr I think I’d be totally OK with not having heterogeneous sequences but instead a JSON type. The only question is how to decide when to use it. Regarding zarr-developers/zarr-extensions#9: I’m not looking forward to adding more blockers to the stack. I planned to finish up this PR stack during the conference, then scanpy 1.13 right after. If they can commit to this next week I’d say that’s fine, but I don’t want to wait in case that issue is asleep for a few more months. |
4d96389 to
7a616bc
Compare
7a616bc to
1265445
Compare
Documentation build overview
159 files changed ·
|
b31066a to
e166165
Compare
flying-sheep
changed the title
|
|
||
| In zarr v3, this is byte-for-byte the layout of the `json` data type ({pr}`zarr-developers/zarr-extensions#9`) | ||
| (which also uses the `vlen-utf8` codec), | ||
| so only the `data_type` metadata will need to change once zarr implementations support it. |
There was a problem hiding this comment.
that can be a v0.1.1 then since it doesn’t change anything about the actual semantics of our spec.
ilan-gold
left a comment
ilan-gold
left a comment
There was a problem hiding this comment.
This raises the question of whether this is a convention or should be a part of our spec (mainly just a notion of interpretation since unlike everything else in our spec, it would break old versions, which this won't)
96d44b9 to
2e0a84c
Compare
flying-sheep
changed the title
ilia-kats
left a comment
ilia-kats
left a comment
There was a problem hiding this comment.
Awesome, thank you!
Unrelated to this PR: A similar design would probably work for writing extra elements of AnnData subclasses (see discussion in scverse/mudata#210).
ee4993c to
d2b142e
Compare
f70dd87 to
5720f48
Compare
ilan-gold
left a comment
ilan-gold
left a comment
There was a problem hiding this comment.
Mostly worried about the language in the spec (thinking of what if TreeData comes along etc.). What if two accessors have the same json schema but different in-memory behaviors? Is that possible? It's a bit pedantic maybe, and maybe non-existent but I often err on the side of caution so I'm just crossing t's and dotting i's here. As it stands now, it's already great :)
| (accessor)= | ||
| ### Accessor specifications (v0.1.0) | ||
|
|
||
| - Each accessor MUST be stored as a scalar {ref}`string <scalars>`, |
There was a problem hiding this comment.
Should we specify that extenders must do this as well or is that clear?
There was a problem hiding this comment.
here too I think that it doesn’t hurt to be explicit. Can be added in the “extending” section probably.
then they still need to set their |
Right, so then (to my point about reserved namespaces), what if two people go for the same name? An additional UUID? That's what |
PyPI is enforcing unique package names, so as long as people name their accessor-project after their package, I think we're fine. |
721c8f3 to
81a869a
Compare
TODO: