Plotting annotations fix - #175

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 Report

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

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

@@            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     

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

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.

@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()

I see, will look into it

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.

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.