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

Do not always reset origin in convert_ROI_table_to_indices #304

Closed
tcompa opened this issue Feb 17, 2023 · 6 comments · Fixed by #305
Closed

Do not always reset origin in convert_ROI_table_to_indices #304

tcompa opened this issue Feb 17, 2023 · 6 comments · Fixed by #305
Labels
Tables AnnData and ROI/feature tables

Comments

@tcompa
Copy link
Collaborator

tcompa commented Feb 17, 2023

At some point while working on #194 (at the level of this commit: 3f2ba78), I observed that the bounding boxes for a certain set of (organoid) labels are wrong, namely they are slightly shifted from the organoids -- see below.

I don't know, yet, if the error is with the bounding boxes or with the way I am using them in the prototype/example.

Screenshot from 2023-02-17 12-35-27

@tcompa
Copy link
Collaborator Author

tcompa commented Feb 17, 2023

First guess: the origin position (that is used to shift ROIs when computing indices, in convert_ROI_table_to_indices) is currently obtained by minimization of some relevant columns in the ROI table, but this is probably not consistent - and it may become especially visible when the table only includes a few very sparse ROIs.

@tcompa
Copy link
Collaborator Author

tcompa commented Feb 17, 2023

First guess: the origin position (that is used to shift ROIs when computing indices, in convert_ROI_table_to_indices) is currently obtained by minimization of some relevant columns in the ROI table, but this is probably not consistent - and it may become especially visible when the table only includes a few very sparse ROIs.

Just as an observation, the current workaround that fixes bounding boxes is to provide an additional (optional) origin_xyz argument to convert_ROI_table_to_indices. In some cases (which ones? for sure the ones where ROIs are sparse objects that do not cover the whole well) we may want to set this by hand, instead of looking for the minimum values.

In the current case, it turns out that during secondary labeling the "correct" value to set is (0,0,0), but it is not immediately clear why. What is clear is that it produces the correct boxes, see below:

Screenshot from 2023-02-17 14-29-59

@tcompa tcompa changed the title Does array_to_bounding_box_table work? Do not always reset origin in array_to_bounding_box_table Feb 20, 2023
@tcompa tcompa changed the title Do not always reset origin in array_to_bounding_box_table Do not always reset origin in convert_ROI_table_to_indices Feb 20, 2023
@tcompa
Copy link
Collaborator Author

tcompa commented Feb 20, 2023

Recap:

  • In convert_ROI_table_to_indices we are (currently) always resetting the origin when computing indices from physical-units ROIs.
  • This is correct as long as ROIs cover the entire well, since the "inferred" origin is always the same for all ROI tables.
  • This is not correct when ROIs define only some sparse regions (e.g. organoids), because the "inferred" origin would be an arbitrary point (the minimal coordinates across all organoids).
  • We fix this issue here via Add reset_origin argument to convert_ROI_table_to_indices (ref #304) #305, by exposing a reset_origin argument in convert_ROI_table_to_indices. The default is True, so that nothing changes in the current behavior. We defer to other issues (like Associate optional mask to each ROI (in tables), and use it for I/O selection #195) the actual choice on when to set reset_origin=False.

@jluethi
Copy link
Collaborator

jluethi commented Feb 20, 2023

Thanks for digging into this @tcompa !
Maybe good to briefly touch on on Wednesday (or at some other call). Not fully sure I understand the finer parts of how we're using these origins and when we'd want to be resetting them.

@tcompa
Copy link
Collaborator Author

tcompa commented Feb 20, 2023

Maybe good to briefly touch on on Wednesday (or at some other call). Not fully sure I understand the finer parts of how we're using these origins and when we'd want to be resetting them.

Sure thing.
I'm moving on with this PR, since for the moment it simply enables a possible change of behavior - without ever activating it in the current tasks. Let's re-discuss it in detail.

@tcompa
Copy link
Collaborator Author

tcompa commented Feb 20, 2023

Briefly, my current understanding is:

  • We do reset the origin when building the well&FOV ROIs, at the beginning of a workflow
  • We should never reset the origin later, e.g. when building label ROIs. We are currently doing it, but probably we shouldn't. And the reason why we never had any problem is that we always used ROIs that cover the entire well.

tcompa added a commit that referenced this issue Feb 20, 2023
…ways-reset-origin-in-array_to_bounding_box_table

Add reset_origin argument to convert_ROI_table_to_indices (ref #304)
@tcompa tcompa mentioned this issue Mar 8, 2023
@tcompa tcompa added the Tables AnnData and ROI/feature tables label Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tables AnnData and ROI/feature tables
Projects
None yet
2 participants