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.

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

RegionDisplay actually also is a data source? #818

Closed
tischi opened this issue Jul 31, 2022 · 16 comments
Closed

RegionDisplay actually also is a data source? #818

tischi opened this issue Jul 31, 2022 · 16 comments

Comments

@tischi
Copy link
Contributor

tischi commented Jul 31, 2022

@constantinpape @martinschorb

I got a bit confused and wonder whether we could improve (at some point) the JSON spec.

RegionDisplay has those fields:

        // annotationId to image sources
	// one annotationId can annotate several images
	protected Map< String, List< String > > sources;

	// table with each row corresponding to one
	// annotationId
	protected Map< TableDataFormat, StorageLocation > tableData;

For comparison, the other place where we have a tableData field is within a source of type segmentation.

My issue is now that RegionDisplay also in fact is a source of some data, namely tableData. Thus, I feel we mixed up the concepts of a data source and a data display. I feel it could be cleaner if we had another source type, maybe called imageRegions (and then we could have another one called spots at some point). Then probably the protected Map< String, List< String > > sources could disappear entirely and simply become one column (e.g., semicolon separated) in the imageRegions table? This could also better fit to what may happen in OME-Zarr where people feel that regions could be specified in a table: ome/ngff#133

And then the RegionDisplay would just specify how to display the regions (and not also hold the actual data). I think this would we more consistent.

What do you think?

@tischi
Copy link
Contributor Author

tischi commented Jul 31, 2022

Naming-wise, maybe even better?
source:imageAnnotation, display:imageAnnotationDisplay

@constantinpape
Copy link
Contributor

I think making regionDisplay a source could make things quite inconsistent, since the table could list sources that are not in the view, leading to undefined behavior.

Naming-wise, maybe even better?
source:imageAnnotation, display:imageAnnotationDisplay

We explicitly decided to not call this annotation any more in order to avoid confusion with the annotation mode (= user provided annotations).

@tischi
Copy link
Contributor Author

tischi commented Aug 1, 2022

OK.... how do we currently plan to model spots? I think you made an example already?!

@constantinpape
Copy link
Contributor

OK....

Regarding the regionDisplay: we can also discuss if it could be expressed as a source, but I don't think it's straightforward and it would probably easier to discuss this in person. But I don't think I have time for this before leaving for vacation (05.08-25.08), but I am happy to discuss this in September.

OK.... how do we currently plan to model spots? I think you made an example already?!

Yes, there is an example project with it already.
https://github.com/mobie/spatial-transcriptomics-example-project/blob/main/data/pos42-spatial-transcriptomics/dataset.json#L60-L66

@tischi
Copy link
Contributor Author

tischi commented Aug 1, 2022

we can also discuss if it could be expressed as a source, but I don't think it's straightforward and it would probably easier to discuss this in person.

I had the same feeling, let's discuss in September. However I think you have a point here, because I think currently the regionDisplay could annotate images that are not part of the sources, but only emerge by means of transformation, in that particular view....

@constantinpape
Copy link
Contributor

However I think you have a point here, because I think currently the regionDisplay could annotate images that are not part of the sources, but only emerge by means of transformation, in that particular view....

Yes, that's one of the subtleties that I think make it difficult to express this as a source.

@martinschorb
Copy link
Contributor

For comparison, the other place where we have a tableData field is within a source of type segmentation.

I have not worked with segmentation sources so far, but to me, the way regionDisplay is designed - referring to the sources (defining the region "position") and then providing the table with the annotation information (color, opacity,...) is intuitive to me.
Maybe things would be more consistent if instead, we move the table reference out of the segmentation source definition and into its Display in the same way we do for regionDisplay.

@constantinpape
Copy link
Contributor

Maybe things would be more consistent if instead, we move the table reference out of the segmentation source definition

Yes, that's a good point we can have in mind! (However it runs a bit contrary to what we have planned for the spot source, see https://github.com/mobie/spatial-transcriptomics-example-project/blob/main/data/pos42-spatial-transcriptomics/dataset.json#L60-L66).

I think it's best if @tischi goes ahead and tries how to fit in the spot source into the current concept, and then we re-evaluate and discuss in person in September (but of course you can keep brainstorming in the meantime!)

@martinschorb
Copy link
Contributor

I think spots (or potentially more general: polygons/vector annotations) are a different story, because the actual positional data resides in the table that the source entry refers to. I think this should be the main criterion, whether an entity is considered a "source", which would be where the basic geometrical information (image size, point positions from table, ...) is stored.

@tischi
Copy link
Contributor Author

tischi commented Aug 1, 2022

Yeah, good points.

I agree that a source is something that makes sense on its own and does not rely on other information.
Thus I agree that spots could it a source.

I find the table that annotates the segments in an image somewhat of an edge case. It does contain relevant information by itself but at the same time is very tightly linked to the label mask image.

We could consider tables being sources, because one always could just display the table by itself, we could then have a simple TableDisplay to show them. SegmentationDisplay and RegionDisplay could use such table data sources to do something with it, e.g. combine a label mask image with a corresponding table. Issue with that: we probably should still store somewhere which tables make sense with which label masks. I think how to express this link is also a discussion here: https://forum.image.sc/t/proposal-for-ome-ngff-table-specification/68908/27
I think there the idea is that a segmentation table MUST contain the information to which image it belongs (I am not sure how this is technically stored in the AnnData object).

@martinschorb
Copy link
Contributor

An alternative could also be to split the generic "tables" into something like "sourceTables" and "annotationTables".
For point/vector sources one could also think of providing the positional data in (zarr) arrays instead. Think for example of super-resolution localisations.

@tischi
Copy link
Contributor Author

tischi commented Sep 26, 2022

@constantinpape @martinschorb

Working now on the spots triggered some more thoughts:

The tableData within the RegionDisplay could be a source, namely a annotationTable (this annotation source could also be viewed just as a table, by means of a TableDisplay)

We add annotations to the spec (same level in the hierarchy as sourceTransformations), one instance being a regionAnnotation

regionAnnotation has the fields:

// regionId to sources:
// sources can be images, segmentations, spots and/or transformed versions thereof.
// one regionId can annotate several sources 
public Map< String, List< String > > sources;

// name of the table source
// containing the regionId column
public String annotationTable;   

the regionAnnotation object fully specifies all the spatial aspects, which allows me to model this in my code as a label mask. What is missing are the display settings, which are then specified in the RegionDisplay where we replace the above two fields by the field:

public String regionAnnotation

So, the RegionDisplay specifies how to display (color, etc) an regionAnnotation.


Pro:

  1. I think this is more logical
  2. It would play better with my code

Con:

  1. The JSON gets longer and we don't gain anything (yet)

In a less invasive version of the above we could also leave out the annotation layer for now but still add the annotationTable to the sources.

@constantinpape
Copy link
Contributor

I can't think this fully through now, but I quite dislike a couple of aspects of this:

  • it makes everything more complex by introducing more elements to the spec without a clear advantage and (at least on the spec level) a more complicated design
  • if we do this then the only logical thing would be to also pull out the segmentation tables -> even more big changes
  • we decouple the table and regionDisplay; where I think we almost always want the table (and if we really think that we don't always want this we could also just make the table field optional)
  • (overall this sounds like several weeks of work which runs counter to the revision timeline)

In a less invasive version of the above we could also leave out the annotation layer for now but still add the annotationTable to the sources.

Yeah, I think this would be fine and a quite logical change (I would call it regionTable though, again if we can let's reserve annotations for the user annotations at least on the spec level).

@tischi
Copy link
Contributor Author

tischi commented Sep 27, 2022

Your comments make sense to me.
Anyway, let's post-pone all of this for after the revision.

@constantinpape
Copy link
Contributor

@tischi: I moved your comments from mobie/mobie.github.io#88 here, since they are more relevant in this issue.

First post

Some other thing that feels a bit confusing.

In the displays, afaik, sources always refers to images or segmentation, but for spots, where it refers to spots sources, which are tables.

Now in the RegionDisplay sources refers to images while the table is referenced via the tableSource field.

In OME-Zarr I guess we will have two main sources of data, namely the spec for images and label mask images, which will be very similar, and the tables.

Thus, in the spotDisplay renaming sources to tableSources would maybe be more consistent?

Second post

@constantinpape @martinschorb

One can also argue the other way around (and looking at my code this seems to make more sense):

In all Displays, but the RegionDisplay the sources are the things that are actually displayed.

Logic: A display can display one or many sources.

So that makes sense and we could keep the name of field sources in SpotDisplay.

Now in the RegionDisplay as I eluded to in the other issue, there is in fact no source that could be readily displayed. The closest thing to a source is in fact the current tableSource field. We could rename it to source, but one could argue not to , because we cannot directly show the table without specifying the images that determine the regions (however, maybe we actually could rename it to source and then just open the table if the images are not given).

Importantly, I would now propose to rename the sources field to regionImages (or something like this). Because a regionDisplay does not show those images; they just provide metadata to the table, namely the position of the regions.

Actually, we could imagine region tables that do contain the full information of where the regions are and could thus be shown self-sustained. If we want to use the same RegionDisplay class for this, then it would also argue to rename tableSource to source, because just the table by itself can be displayed.

Any thoughts?

@constantinpape
Copy link
Contributor

We now have regions as a source, which holds the table for the regionDisplay. I think this is the most consistent solution within the spec, so I will go ahead and close this for now, But feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants