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
Table spec proposal #64
Table spec proposal #64
Changes from 24 commits
4559ce5
c9148a7
d3330a2
957555f
f8f2fd0
83c9491
8d2d64e
d715323
98be5fa
bc72166
f4a13c6
ffdd046
0d5636e
1d59ca4
d91da64
4ad6a90
fac5855
e3f779c
75fb5e1
00e85c1
5d0b5b4
fe2cfad
692464f
cc83a82
b2de201
283d21b
bf81797
4b46882
f3e960b
ad48296
55fb1a8
337971b
e9f707b
22844a4
ea8a622
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.
In the example at https://github.com/kevinyamauchi/ome-ngff-tables-prototype/blob/4fdde521b6b5514424f9c6508a8b1fc3a2cff86e/src/ngff_tables_prototype/writer.py#L248 this group is called "tables" with path of
tables/regions_table/.zattrs
whereas here it is justtable/.zattrs
(without the nestedregions_table
group.)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.
Sorry for the confusion! As specified, the group containing a given table can either be in root or inside of another group.
In the
ome-ngff-tables-prototype
example, the table calledregions_table
is within a group calledtables
. In this case, one could have multiple tables (e.g.,regions_table_1
, andregions_table_2
) within thetables
group. As written here, the table is called itable
and is stored in root.I can see how the usage of
table
here is confusing. Should I rename the table to something else or maybe just specify in the comment that this is a table stored in root?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.
In the 'labels' case the
image.zarr/labels/.zattrs
contains a list of child labels, e.g.This allows you to find the child labels without having to
ls
to find all child directories (which you can't do on s3 or http etc.).So we should do something similar with tables...
image.zarr/tables/.zattrs
lists one or more tables: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 have implemented this proposal in kevinyamauchi/ome-ngff-tables-prototype#12
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 have added this to the spec here: f3e960b
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.
Thanks for the update @kevinyamauchi. However, that's not quite the same as what we have for labels. In the above suggestions (and for labels) we have an extra level of the hierarchy that is not what's currently in this PR:
image.123/tables/my_table/
whereimage.123/tables/.zattrs
has{"tables": ["my_table"]}
.In this case, if there are any tables for the
image.123
thenimage.123/tables/.zattrs
should exist.Currently this PR has
image.123/table/
whereimage.123/.zattrs
has{"tables": ["table"]}
.So the note about
.zattrs
listing "tables" needs to be moved one level lower in the hierarchy, and renameimage.123/table
toimage.123/tables
.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.
Hi @will-moore , I think there shouldn't be problems here, although @ivirshup and @kevinyamauchi should confirm. I made a PR on kevin's branch here: kevinyamauchi#1 is that you had in mind?
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.
Sorry for the delay, @will-moore . I have merged @giovp 's PR, which should address this issue.
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.
this is really cool to see, and it'd be really important to have this flexibility of having tables both in root and in image/labels group.
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.
Points haven't been defined yet.
Also, I'm not completley sure what this means given the bioformats2raw layout and the fact we can now put the tables group on its own. Maybe we should remove this?
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.
Good point. I removed the references to points and changed it to say that the tables group may be in root or another group.
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 would use a term different from
ngff:region_table
, maybengff:annotation_table
, orngff:table
. The reason is that the name may suggest that this table is describing regions, while it is instead annotating regions.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 that this
MUST
is too strong for two reasons.region
,region_key
andinstance_key
specified, so relaxing this would make this table future proof, and be a general table that is the base for other types of tables (for instance annotating collection of images/sample, describing points/shapes, etc). This is also the reason why I would call this tablengff:table
. An alternative is to make this table specific exactly to annotating labels, and then relax this in further specifications, but I think that people would start using the table for more general uses anyway.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.
+1 for relaxing the constraint of having an associated region, region_key & instance_key. We also use tables both for region annotation (e.g. feature measurements) and for defining regions of interest in intensity images (=> no label image exists)
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 would be more correct to use the terms
regions
,regions_key
,instance_key
(notice the plural for regions), this because Labels (and in the future Points, Circles, Polygons, ...) are objects that describe multiple regions, not just one region per object. So sentences like the one above"region" MUST be a single path (single region) or ...
are misleading, because there is not a single region, but multiple regions, or a single regions object.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.
@bogovicj , @joshmoore , does this track with your understanding of our proposal for paths?
Is the plan to add a section somewhere on paths that can be referenced?
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.
Current example at https://github.com/kevinyamauchi/ome-ngff-tables-prototype/blob/4fdde521b6b5514424f9c6508a8b1fc3a2cff86e/src/ngff_tables_prototype/writer.py#L60 generates a string e.g, "labels/label_image", not an object with "path".
Also, it'll be easier to validate etc if we have a list with a single entry for single
region
instead of a "single path or array of paths", since then we have to handle 2 different types of value.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.
Comment related to the paths. As you pointed out in the presentation, the column named after the value of
region_key
could be either a full/relative path/url (string) either an index relative to the paths described inregion
. Maybe the second would be preferable. Also it could be a workaround for cases in which the same row of a table is annotating multiple regions, as mentioned in this pr discussion and developed more in details here scverse/spatialdata#34There 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.
This review comment was from months ago (forgot to send), I prefer the full path approach since it is less ambiguous, and the workaround for rows annotating multiple regions can still be achieved with full paths, as I described in this comment: #64 (comment)
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 we have used relative paths everywhere else in the NGFF spec to date. This is preferable in the case that you generate the data in one location, then upload it to a different location/server which you don't know about when you generate the data.
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 don't see where
obsm_0
is referenced in the parent.zattrs
?How do we know to load the
obsm_0
data (if we can'tls
the directories belowobsm
)?The
obs/.zattrs
has"column-order"
which lists columns, but there isn't an equivalent forobsm/.zattrs
.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.
Same issue exists for
obsp
,varm
andvarp
?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.
We've basically assumed that you can
ls
, and not maintained order for these.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.
Unfortunately that's not the case if you're loading data over
https
(or for somes3
backends), so we've avoided making that assumption elsewhere on the spec to allow web-based accessing of OME-NGFF.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.
👍 for the locations of everything in the hierarchy being either computable (implementations know how to figure it out) or explicitly listed in the metadata.
FWIW, the OME2022 zarr-java discussion today touched on exactly this point and the need to perhaps bubble this requirement up to the zarr spec itself.
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.
Let's consider the 2 issues separately, since I think the cost/benefit of consolidated metadata are different...
For the top-level
image.zarr/tables/.zattrs
it is very easy to manually add the metadata to list the child tables as in kevinyamauchi/ome-ngff-tables-prototype#12. This should be a requirement (MUST) in the spec so all readers can rely on it (without needing to try consolidated metadata).However, in the other case, I don't know how easy it is to adopt a similar approach for the
obsm, obsp, layers, raw and uns
groups? I'm not very familiar with AnnData and haven't dug into the creation of these groups.Using a naive approach, I tried simply listing the sub-directories of these groups, and adding those names to the
.zattrs
of each group.E.g. add this to
write_table_regions()
from theome-ngff-tables-prototype
:E.g.
obsm/.zattrs
now looks like this:This approach feels a bit hacky, and would need to be recursive in some cases (e.g. uns).
I don't have a strong preference for this compared with
consolidate_metadata()
.These sets of metadata aren't the core matrix data for the ann-data table. They're not even displayed in napari (in the screenshot) so it's less critical than listing the tables above.
If a client relies on
consolidate_metadata()
and it's not available, then they simply wouldn't display this metadata.A client wouldn't need to always try with and without consolidated metadata.
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.
Just a quick side note on the viewer, the new
napari-spatialdata
, that we haven't involved in the discussion since atm it needs some bugfix and its scope is beyond displaying annotated tables (it experiments with points and polygons as well), shows by default all those entries (obsm
,obs
, etc), with the exception ofuns
.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.
Having data disappear because of something that did or did not get called in the python code and is otherwise not recorded doesn't feel 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.
@joshmoore So you prefer the
# write group names into .zattrs
approach above (like a manual consolidate metadata)? Or another alternative?The presence or absence of everything in the viewers is always depends on whether the creating code did or did not add something. That's the choice that the code has when the spec says
SHOULD
orMAY
.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.
All other things being equal, I prefer that all details & members of a fileset are either well-known beforehand (i.e. static in the spec) or can be determined from the metadata.