Plotting annotations fix #175

melonora · 2023-10-26T16:00:40Z

closes #164

This PR fixes the issue reported in #164. The problem was that when filtering the anndata object it would result in an anndata object with potentially 0 rows. This PR checks whether the anndata object to be added to the metadata has 0 rows. If this is the case then the adata metadata is set to be None. Also if there is no table, adata will be none, but in addition region_key will also be None.

In addition to the implementation of @aeisenbarth this handles the case when a table is present, but there are no annotations for a given element.

melonora · 2023-10-26T17:44:35Z

I added a test that integrates the sdata_widget and adata view widget. Not certain whether we want to move these in a particular file or whether this is ok.

codecov-commenter · 2023-10-31T08:52:52Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (b906ee2) 75.79% compared to head (fa33e62) 76.84%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #175      +/-   ##
==========================================
+ Coverage   75.79%   76.84%   +1.04%     
==========================================
  Files          16       16              
  Lines        1719     1723       +4     
==========================================
+ Hits         1303     1324      +21     
+ Misses        416      399      -17

Files	Coverage Δ
src/napari_spatialdata/_view.py	`80.88% <ø> (+1.35%)`	⬆️
src/napari_spatialdata/_viewer.py	`72.41% <100.00%> (+8.32%)`	⬆️
src/napari_spatialdata/utils/_utils.py	`78.01% <85.71%> (+0.83%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

LucaMarconato · 2023-11-05T15:01:01Z

src/napari_spatialdata/_view.py

@@ -252,18 +252,10 @@ def _select_layer(self) -> None:
                self.model.adata.obsm[Key.obsm.spatial][:, ::-1][:, :2], 0, values=0, axis=1
            )

-        if "points" in layer.metadata:


LucaMarconato · 2023-11-05T15:02:01Z

src/napari_spatialdata/_viewer.py

-                "adata": sdata.table[sdata.table.obs[sdata.table.uns["spatialdata_attrs"]["region_key"]] == key]
-                if sdata.table
-                else None,
+                "adata": adata,
                "region_key": sdata.table.uns["spatialdata_attrs"]["instance_key"] if sdata.table else None,


in lines 169 and 214 we have "region_key" on the right hand side, here we have "instance_key", I think this is a bug.

Could this instance_key be the cause of the bug with reversed indices? See my comment in the discussion page.

LucaMarconato · 2023-11-05T15:04:37Z

src/napari_spatialdata/utils/_utils.py

@@ -367,3 +367,18 @@ def get_elements_meta_mapping(
                name_to_add = name
            elements[name] = elements_metadata
    return elements, name_to_add
+
+
+def _get_metadata_adata(sdata: SpatialData, key: str) -> None | AnnData:


Nice to have this separation, it makes the code more readable. Since there is a bug with the order of the rows (see my comment in the discussion), we could fix it by replacing this function with match_table_to_element() https://github.com/scverse/spatialdata/blob/bf3377fd36f94ef51a233ae71312b1c28e978e6e/src/spatialdata/_core/query/relational_query.py#L134, which returns a table with the correct order.

LucaMarconato · 2023-11-05T15:04:53Z

tests/test_viewer.py

+    assert viewer.layers[-1].metadata["adata"]
+    assert view_widget.obs_widget.item(0)
+
+    # Filtering adata leads to 0 rows so adata should be set to None


LucaMarconato · 2023-11-05T15:11:13Z

Thanks @melonora for the fix! The bug that I have reported in #164 is gone.

During my review I have anyway spotted two parts of the code to double check.

One is the use of a instance_key when in other equivalent parts of the code region_key was used.

The other is that I noticed that the current implementation is not robust against reordering of the rows of the table (as I show in the code below). I think that this can be fixed by replacing the function _get_metadata_adata() with a call of match_table_to_element() (https://github.com/scverse/spatialdata/blob/bf3377fd36f94ef51a233ae71312b1c28e978e6e/src/spatialdata/_core/query/relational_query.py#L134), which returns a table where the rows are reordered to match the element. Anyway, I can't exclude that the bug is caused by the strange use of instance_key that I commented above.

Here is the code to reproduce the behavior. The table is essentially the same, but the two visualizations are different.

##
from napari_spatialdata import Interactive
from spatialdata.datasets import blobs

sdata0 = blobs()
table0 = sdata0.table

table1 = table0.copy()
table1.obs = table1.obs.iloc[::-1]
sdata1 = blobs()
del sdata1.table
sdata1.table = table1

print(table0.obs['instance_id'])
print(table1.obs['instance_id'])
##
Interactive(sdata0)
Interactive(sdata1)

sdata0:

sdata1:

On this topic, @kevinyamauchi @giovp I think this bug is a symptom that we should drop region, region_key, instance_key altogether and just use the obs indices, and by limiting tables to annotate 0 or 1 spatial element, but not more than 1.

LucaMarconato · 2023-11-05T15:24:48Z

@melonora I have checked if this happens for circles and it seems it doesn't: the two plots are identical when sorting the rows. So it seems that the bug is not do to not using match_table_to_element() but to the use of instance_key where there should be region_key. I haven't verified this further yet.

I have expanded the code above to check also circles, also with spatialdata-plot (doesn't work atm because of unrelated bugs).

##
from napari_spatialdata import Interactive
from spatialdata.datasets import blobs
import pandas as pd

# the table annotates labels
sdata0 = blobs()
table0 = sdata0.table

# the table annotates labels, but by reversing the table rows, the plot should be identical
table1 = table0.copy()
table1.obs = table1.obs.iloc[::-1]
sdata1 = blobs()
del sdata1.table
sdata1.table = table1

print(table0.obs['instance_id'])
print(table1.obs['instance_id'])
##

# the table annotates the circles
table2 = table0.copy()[:5]
table2.obs = pd.DataFrame(table2.obs)
table2.uns['spatialdata_attrs']['region'] = 'blobs_circles'
table2.obs['region'] = 'blobs_circles'
table2.obs['instance_id'] = list(range(5))
sdata2 = blobs()
del sdata2.table
sdata2.table = table2

# the table annotates the circles, but by reversing the table rows, the plot should be identical
table3 = table2.copy()
table3.obs = table3.obs.iloc[::-1]
sdata3 = blobs()
del sdata3.table
sdata3.table = table3

print(table2.obs['instance_id'])
print(table3.obs['instance_id'])

##
Interactive(sdata0)
Interactive(sdata1)
Interactive(sdata2)
Interactive(sdata3)

##
import spatialdata_plot

# the two plots should be identical; I bet it's true, but I can't check it because of a bug
sdata0.pl.render_labels(color='channel_0_sum').pl.show()  # doesn't work because
sdata1.pl.render_labels(color='channel_0_sum').pl.show()

# the two plots should be identical; I bet it's true, but I can't check it because there is another bug
sdata2.pl.render_shapes(color='channel_0_sum').pl.show()
sdata3.pl.render_shapes(color='channel_0_sum').pl.show()

import matplotlib.pyplot as plt
plt.show()

melonora · 2023-11-05T15:35:43Z

I see, will look into it

LucaMarconato · 2023-11-05T21:18:14Z

Still trying to understand the reasons for the behavior observed above. Sharing some initial considerations.

in _model.py, we should drop labels_key and call it instance_key https://github.com/melonora/napari-spatialdata/blob/fa33e6292b668b38ce7a7f2058740c989857ae08/src/napari_spatialdata/_model.py#L268;
labels_key is indeed used only for labels, as per this line self.model._labels_key = layer.metadata["region_key"] if isinstance(layer, Labels) else None from _view.py; https://github.com/melonora/napari-spatialdata/blob/fa33e6292b668b38ce7a7f2058740c989857ae08/src/napari_spatialdata/_view.py#L256
when we call region_key=.... instance_key... in line https://github.com/melonora/napari-spatialdata/blob/fa33e6292b668b38ce7a7f2058740c989857ae08/src/napari_spatialdata/_viewer.py#L234C8-L234C8, the error is not on the right hand side, but it is in the function argument, which should be called instance_key. Currently the region_key value is assigned to labels_key in _model.py https://github.com/melonora/napari-spatialdata/blob/fa33e6292b668b38ce7a7f2058740c989857ae08/src/napari_spatialdata/_model.py#L273
There is no place where the instance_key/labels_key is used to sort the rows of the table, so no surprise that after permuting the rows of the table the plots are wrong. But for some reasons for circles the plots is correct after permuting the rows. I haven't yet understood why.
We should do a deep cleaning of _model.py, it's full of legacy code that makes things messy.

LucaMarconato · 2023-11-05T21:24:51Z

Let's merge this PR as it is, since it fixes the case empty tables, and break the problem in two smaller ones. Let's make a PR for cleaning up _models.py and after that let's fix the plotting when the rows of the table are shuffled.

melonora added 5 commits October 26, 2023 13:41

Points never in metadata

8a9a4bb

colormap not in metadata

d8953fe

add adata utility function

160e32d

add test adata metadata

b25cb07

integrated view_widget test

6c214f4

melonora requested a review from LucaMarconato October 26, 2023 17:43

melonora marked this pull request as ready for review October 26, 2023 17:43

melonora added 3 commits October 31, 2023 08:42

add condition if no sdata table

e9f7fbc

Change region key

994edda

Merge branch 'main' into plotting_annotations

fa33e62

LucaMarconato reviewed Nov 5, 2023

View reviewed changes

LucaMarconato mentioned this pull request Nov 5, 2023

Bug: plotting after sorting a table scverse/spatialdata-plot#178

Open

LucaMarconato mentioned this pull request Nov 5, 2023

pl.show() still not working (?) in interactive mode scverse/spatialdata-plot#68

Open

LucaMarconato merged commit a3d6189 into scverse:main Nov 5, 2023

LucaMarconato mentioned this pull request Nov 5, 2023

The plot should be invariant against reordering of the table rows #181

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plotting annotations fix #175

Plotting annotations fix #175

melonora commented Oct 26, 2023 •

edited

Loading

melonora commented Oct 26, 2023

codecov-commenter commented Oct 31, 2023 •

edited

Loading

LucaMarconato Nov 5, 2023

LucaMarconato Nov 5, 2023

LucaMarconato Nov 5, 2023

LucaMarconato Nov 5, 2023

LucaMarconato Nov 5, 2023

LucaMarconato commented Nov 5, 2023

LucaMarconato commented Nov 5, 2023 •

edited

Loading

melonora commented Nov 5, 2023

LucaMarconato commented Nov 5, 2023 •

edited

Loading

LucaMarconato commented Nov 5, 2023

Plotting annotations fix #175

Plotting annotations fix #175

Conversation

melonora commented Oct 26, 2023 • edited Loading

melonora commented Oct 26, 2023

codecov-commenter commented Oct 31, 2023 • edited Loading

Codecov Report

LucaMarconato Nov 5, 2023

Choose a reason for hiding this comment

LucaMarconato Nov 5, 2023

Choose a reason for hiding this comment

LucaMarconato Nov 5, 2023

Choose a reason for hiding this comment

LucaMarconato Nov 5, 2023

Choose a reason for hiding this comment

LucaMarconato Nov 5, 2023

Choose a reason for hiding this comment

LucaMarconato commented Nov 5, 2023

LucaMarconato commented Nov 5, 2023 • edited Loading

melonora commented Nov 5, 2023

LucaMarconato commented Nov 5, 2023 • edited Loading

LucaMarconato commented Nov 5, 2023

melonora commented Oct 26, 2023 •

edited

Loading

codecov-commenter commented Oct 31, 2023 •

edited

Loading

LucaMarconato commented Nov 5, 2023 •

edited

Loading

LucaMarconato commented Nov 5, 2023 •

edited

Loading