Skip to content
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.

Sign up for GitHub

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

Closed
wants to merge 35 commits into from
Closed

Table spec proposal #64

Changes from 9 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
4559ce5
add tables spec draft
kevinyamauchi Oct 1, 2021
c9148a7
X must be a single dtype
kevinyamauchi Oct 1, 2021
d3330a2
add points spec draft
kevinyamauchi Oct 1, 2021
957555f
add spec for axis names
kevinyamauchi Oct 20, 2021
f8f2fd0
remove points spec
kevinyamauchi Jun 19, 2022
83c9491
Merge branch 'main' into add-tables-points
kevinyamauchi Jun 19, 2022
8d2d64e
add layers
kevinyamauchi Jun 19, 2022
d715323
add varm/obsm
kevinyamauchi Jun 19, 2022
98be5fa
add obsp/varp
kevinyamauchi Jun 19, 2022
bc72166
add missing .zarray
kevinyamauchi Jun 19, 2022
f4a13c6
add @type
kevinyamauchi Jun 19, 2022
ffdd046
Apply suggestions from code review
kevinyamauchi Jun 20, 2022
0d5636e
update uns
kevinyamauchi Jun 20, 2022
1d59ca4
add parent
kevinyamauchi Jun 20, 2022
d91da64
MAY -> MUST for anndata encoding
kevinyamauchi Jun 20, 2022
4ad6a90
indices, indptr dtype
kevinyamauchi Jun 20, 2022
fac5855
csc matrix
kevinyamauchi Jun 20, 2022
e3f779c
update table metadata
kevinyamauchi Jun 20, 2022
75fb5e1
Update index.bs
kevinyamauchi Jun 20, 2022
00e85c1
Update latest/index.bs
kevinyamauchi Jun 20, 2022
5d0b5b4
Update latest/index.bs
kevinyamauchi Jun 20, 2022
fe2cfad
update var table
kevinyamauchi Jun 29, 2022
692464f
update path spec
kevinyamauchi Oct 8, 2022
cc83a82
typo
kevinyamauchi Oct 8, 2022
b2de201
Apply suggestions from @ivirshup
kevinyamauchi Oct 28, 2022
283d21b
clarify allowed locations
kevinyamauchi Oct 28, 2022
bf81797
remove intensity image metadata
kevinyamauchi Oct 28, 2022
4b46882
remove reference to image id
kevinyamauchi Nov 15, 2022
f3e960b
add tables metadata
kevinyamauchi Jan 8, 2023
ad48296
add list of keys for subgroups
kevinyamauchi Jan 8, 2023
55fb1a8
add tables
giovp Feb 22, 2023
337971b
rename table
giovp Feb 22, 2023
e9f707b
add my_table consistently
giovp Feb 22, 2023
22844a4
remove comment
giovp Feb 23, 2023
ea8a622
Merge pull request #1 from giovp/add-tables-points
kevinyamauchi Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 167 additions & 0 deletions latest/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,174 @@ For this example we assume an image with 5 dimensions and axes called `t,c,z,y,x
└── n
</pre>

Tables {#table-layout}
----------------------
The following describes the expected layout for tabular data. OME-NGFF tables are a compatible subset of the [AnnData model](https://github.com/theislab/anndata). OME-NGFF tables implement the X (central dense matrix), the obs table (annotations on the rows of X) and the var table (annotations on the columns of X).
kevinyamauchi marked this conversation as resolved.
Show resolved Hide resolved

<pre>
. # Root folder, potentially in S3,
│ # with a flat list of images by image ID.
will-moore marked this conversation as resolved.
Show resolved Hide resolved
│
└── 123.zarr
└── table # The table group is a container which holds a table that is compatible with a subset of AnnData.
kevinyamauchi marked this conversation as resolved.
Show resolved Hide resolved
│ # The table group MAY be in the root of the zarr file.
├── .zgroup # The table group MAY be in an image, labels, or points group.
Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
├── .zgroup # The table group MAY be in an image, labels, or points group.
├── .zgroup # The table group MAY be in an image, labels, or points group.

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?

Copy link
Contributor Author

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.

├── .zattrs
│
├── X # You MAY add an zarr array `X`.
│ │ # `X` MUST not be a complex type (i.e., MUST be a single type)
│ │ # `X` MAY be chunked as the user desires.
│ ├── .zarray
│ ├── 0.0
│ │ ...
│ └── n.m
|
├── layers # You MAY add a `layers` group, which contains dense matrices with the same shape as X.
│ │
│ ├── .zgroup
│ ├── .zattrs
│ │
│ └── layer_0 # You MAY add a zarr array for each layer
| | # Each layer array MUST have the same shape as X
| | # Each layer array SHOULD be chunked the same as X
| ├── 0.0
│ │ ...
│ └── n.m
│
├── obs # You MAY add an obs group container. The obs group holds a table of annotations on the rows in X.
│ │ # The rows in obs MUST be index-matched to the rows in X.
│ ├── .zgroup
│ │
│ ├── .zattrs # `.zattrs` MUST contain `"_index"`, which is the name of the column in obs to be used as the index.
│ │ # `.zattrs` MUST contain `"column-order"`, which is a list of the order of the non-_index columns.
│ │ # `.zattrs` MAY contain `"encoding-type"`, which is set to `"dataframe"` by AnnData.
│ │ # `.zattrs` MAY contain `"encoding-version"`, which is set to `"0.1.0"` by AnnData.
│ │
│ └── col_0 # Each column in the obs table is a 1D zarr array. The rows can be chunked as the user desires.
kevinyamauchi marked this conversation as resolved.
Show resolved Hide resolved
│ ├── .zarray # However, the obs columns SHOULD be chunked in the same way as the rows in X (if present).
│ │
│ └─ 0
├── var # You MAY add a var group container. The var group holds a table of annotations on the columns in X.
| │ # The rows in var MUST be index-matched to the columns in X (if present).
| |
| ├── .zattrs # `.zattrs` MUST contain `"_index"`, which is the name of the column in obs to be used as the index.
| │ # `.zattrs` MUST contain `"column-order"`, which is a list of the order of the non-_index columns.
| │ # `.zattrs` MAY contain `"encoding-type"`, which is set to `"dataframe"` by AnnData.
| │ # `.zattrs` MAY contain `"encoding-version"`, which is set to `"0.1.0"` by AnnData.
| │
| └── col_0 # Each column in the var table is a 1D zarr array. The rows can be chunked as the user desires.
| ├── .zarray # However, the var columns SHOULD be chunked in the same way as the columns in X.
| │
| └─ 0
|
├── obsm # You MAY add a obsm group comtainer. The obsm group contains arrays that annotate the rows in X.
| │ # The rows in each array MUST be index-matched to the rows in X (if present).
| |
│ ├── .zgroup
| |
| ├── .zattrs # `.zattrs` MAY contain `"encoding-type"`, which is set to `"dict"` by AnnData.
| │ # `.zattrs` MAY contain `"encoding-version"`, which is set to `"0.1.0"` by AnnData.
| │
│ └── obsm_0 # You MAY add a zarr array for each obsm matrix.
Copy link
Member

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't ls the directories below obsm)?
The obs/.zattrs has "column-order" which lists columns, but there isn't an equivalent for obsm/.zattrs.

Copy link
Member

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 and varp?

Copy link
Contributor

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.

Copy link
Member

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 some s3 backends), so we've avoided making that assumption elsewhere on the spec to allow web-based accessing of OME-NGFF.

Copy link
Member

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.

Copy link
Member

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 the ome-ngff-tables-prototype:

    # write group names into .zattrs
    table_path = os.path.join(group.store.dir_path(), group.path, table_group_name)
    for sub_dir in ["layers", "obsm", "obsp", "raw", "uns"]:
        sub_path = os.path.join(table_path, sub_dir)
        if not os.path.exists(sub_path):
            continue
        children = [f.name for f in os.scandir(sub_path) if f.is_dir()]
        sub_group = table_group[sub_dir]
        sub_group.attrs[sub_dir] = children

E.g. obsm/.zattrs now looks like this:

{
    "encoding-type": "dict",
    "encoding-version": "0.1.0",
    "obsm": [
        "X_scanorama",
        "X_umap",
        "spatial"
    ]
}

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.

Copy link

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 of uns.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a client relies on consolidate_metadata() and it's not available, then they simply wouldn't display this metadata.

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.

Copy link
Member

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 or MAY.

Copy link
Member

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.

| | # Each obsm array MUST have the same number of rows as X.
| | # The rows in each obsm array SHOULD be chunked the same as the rows in X.
kevinyamauchi marked this conversation as resolved.
Show resolved Hide resolved
| ├── 0.0
│ │ ...
│ └── n.m
|
├── varm # You MAY add a varm group comtainer. The varm group contains arrays that annotate the columns in X.
| │ # The rows in each array MUST be index-matched to the columns in X (if present).
| |
│ ├── .zgroup
| |
| ├── .zattrs # `.zattrs` MAY contain `"encoding-type"`, which is set to `"dict"` by AnnData.
| │ # `.zattrs` MAY contain `"encoding-version"`, which is set to `"0.1.0"` by AnnData.
kevinyamauchi marked this conversation as resolved.
Show resolved Hide resolved
| │
│ └── varm_0 # You MAY add a zarr array for each varm matrix.
| | # Each varmm array MUST have the same number of rows as columns in X.
kevinyamauchi marked this conversation as resolved.
Show resolved Hide resolved
| | # The rows in each obsm array SHOULD be chunked the same as the columns in X.
| ├── 0.0
│ │ ...
│ └── n.m
|
├── obsp # You MAY add a obsp group comtainer. The obsp group contains sparse arrays that annotate the rows in X.
| │ # The rows in each array MUST be index-matched to the columns in X (if present).
| |
│ ├── .zgroup
| |
| ├── .zattrs # `.zattrs` MAY contain `"encoding-type"`, which is set to `"dict"` by AnnData.
| │ # `.zattrs` MAY contain `"encoding-version"`, which is set to `"0.1.0"` by AnnData.
| │
│ └── obsp_0 # You MAY add a zarr group for each obsp array.
| | # Each obsp array MUST have the same number of rows as rows in X.
| |
│ ├── .zgroup
| |
| ├── .zattrs # `.zattrs` MAY contain `"encoding-type"`, which is set to `"csr_matrix"` by AnnData.
kevinyamauchi marked this conversation as resolved.
Show resolved Hide resolved
| │ # `.zattrs` MAY contain `"encoding-version"`, which is set to `"0.1.0"` by AnnData.
| | # `.zattrs` MAY contain `"shape"` which is an array giving the shape of the densified array.
| |
| ├── data # You MUST add a one-dimensional zarr array named "data".
| | | # `data` MAY be chunked as the user desires.
| | |
| | ├── 0
│ │ | ...
│ | └── n
| |
| ├── indices # You MUST add a one-dimensional zarr array named "indices".
| | | # `indices` MAY be chunked as the user desires.
| | |
| | ├── 0
│ │ | ...
│ | └── n
| |
| └── indptr # You MUST add a one-dimensional zarr array named "indptr".
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivirshup, do you think we should provide more detail on what should be in data, indices, and indptr?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indices and indptr must be ints.

Were you thinking of more?

Copy link
Contributor Author

@kevinyamauchi kevinyamauchi Jun 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point regarding indices and indptr, @ivirshup.

I was also wondering if there is a spec we should reference or if saying csc/csr sparse matrix is enough for people to know what to put in data/indicies/indptr (I don't work with sparse matrices a ton myself)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just seeing I didn't respond here!

We could link out to wikipedia for a definition?

I think it would be hard to have a concise description that would explain it for someone who's totally unfamiliar, and a full description would be kinda long.

Maybe:

  • data: array containing values of the non-zero entries
  • indices column/ row indices of the non-zero entries (for CSR/ CSC respectively)
  • indptr cumulative offsets for row/ columns indices (CSR/ CSC respectively)

| | # `indptr` MAY be chunked as the user desires.
| |
| ├── 0
│ | ...
│ └── n
|
├── varp # You MAY add a varp group comtainer. The varp group contains sparse arrays that annotate the columns in X.
| │ # The rows in each array MUST be index-matched to the columns in X (if present).
| |
│ ├── .zgroup
| |
| ├── .zattrs # `.zattrs` MAY contain `"encoding-type"`, which is set to `"dict"` by AnnData.
| │ # `.zattrs` MAY contain `"encoding-version"`, which is set to `"0.1.0"` by AnnData.
| │
│ └── varp_0 # You MAY add a zarr group for each varp array.
| | # Each varp array MUST have the same number of rows as columns in X.
| |
│ ├── .zgroup
| |
| ├── .zattrs # `.zattrs` MAY contain `"encoding-type"`, which is set to `"csr_matrix"` by AnnData.
| │ # `.zattrs` MAY contain `"encoding-version"`, which is set to `"0.1.0"` by AnnData.
| | # `.zattrs` MAY contain `"shape"` which is an array giving the shape of the densified array.
| |
| ├── data # You MUST add a one-dimensional zarr array named "data".
| | | # `data` MAY be chunked as the user desires.
| | |
| | ├── 0
│ │ | ...
│ | └── n
| |
| ├── indices # You MUST add a one-dimensional zarr array named "indices".
| | | # `indices` MAY be chunked as the user desires.
| | |
| | ├── 0
│ │ | ...
│ | └── n
| |
| └── indptr # You MUST add a one-dimensional zarr array named "indptr".
| | # `indptr` MAY be chunked as the user desires.
| |
| ├── 0
│ | ...
│ └── n
└── uns
kevinyamauchi marked this conversation as resolved.
Show resolved Hide resolved

</pre>

High-content screening {#hcs-layout}
------------------------------------
Expand Down