get_element_instances and get_centroids should not (?) return zero element #614

Activity

1. giovp commented on Jul 4, 2024

   

   giovp

   on Jul 4, 2024

   Last edited by giovp

   MemberAuthor

   More actions

   I think we also don't allow to have "0" as a value in the index of shapes, but I am not entirely sure about that @LucaMarconato ? ~~ If not, I think we should probably also enforce that in order to avoid misunderstanding between instances retrieved from labels or shapes, wdyt? ~~

   EDIT: we don't enforce it, I wonder, should we?
2. giovp commented on Jul 4, 2024

   

   giovp

   on Jul 4, 2024

   Last edited by giovp

   MemberAuthor

   More actions

   same here I think, what is the "centroid" value of an empty space in a label?

   spatialdata/src/spatialdata/_core/centroids.py

   Line 77 in f94d0c7

   u = da.unique(portion, return_counts=True)
3. giovp

   changed the title [-]`get_element_instances`should not (?) return zero[/-] [+]`get_element_instances` and `get_centroids` should not (?) return zero element[/+] on Jul 4, 2024
4. LucaMarconato commented on Jul 5, 2024

   

   LucaMarconato

   on Jul 5, 2024

   Last edited by LucaMarconato

   Member

   More actions

   I agree, we should probably not return it for Labels, and for shapes it could indeed be confusing to have it. The problem is that for shapes there are some readers where we set instance_key to np.arange(len(shapes)) (as opposed to have the string values that were default). So, if we set an error for shapes we would break the compatibility with (at least) Visium.

   I suggest the following:

   • add an optional argument to get_centroids() include_background: bool = False to ignore the background for labels. This will close the current issue.
   • for the moment, or for good, keep allowing 0 for shapes.

   In this way, the inconvenient case that we leave uncovered is when a user takes some shapes data that has instances named 0 (such as Visium), and rasterizes the shapes into labels. In this way in fact the shapes 0 will turn into the background.
5. LucaMarconato commented on Jul 5, 2024

   

   LucaMarconato

   on Jul 5, 2024

   Member

   More actions

   I suggest to open a separate issue to discuss a strategy to deal with 0 in shapes and points (we can add an intro to this coment and turn it into an issue):

   • remove all the np.arange(len(...)) from spatialdata-io and spatialdata-sandbox and make the indices start from 1 instead.

   This keeps the shapes<>tables consistent but create one problem: if the user parsed some data with an earlier version of spatialdata-io and had hardcoded some indices, now the indices are shifted. Not sure what's the best way to deal with this.
6. giovp commented on Jul 8, 2024

   

   giovp

   on Jul 8, 2024

   MemberAuthor

   More actions

   thanks for the comments,

   for the moment, or for good, keep allowing 0 for shapes.

   I think I agree with this, enforcing the non-zero index for shapes might be a bit too much, not only for compatibility changes, but also simply because it is a spatialdata-specific case that it's maybe a bit-exotic and not particularly useful. I will open a PR to change the get_centroids result to remove the 0.
7. giovp

   closed this as completedin #621on Jul 9, 2024