Skip to content

add argument return_background for get_centroids and get_element_instances in the case of labels#621

Merged
giovp merged 7 commits intomainfrom
giovp/get_centroids_label
Jul 9, 2024
Merged

add argument return_background for get_centroids and get_element_instances in the case of labels#621
giovp merged 7 commits intomainfrom
giovp/get_centroids_label

Conversation

@giovp
Copy link
Member

@giovp giovp commented Jul 8, 2024

closes #614

@giovp giovp requested a review from LucaMarconato July 8, 2024 09:10
)
assert all(table.obs["instance_id"] == range(100))

assert all(table.obs["instance_id"] == range(1, 100))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here the labels has values 0,100 but the 0 element is background, so is effectively filtered in the left/right joins when get_element_instances is called on labels. Hence the indices of the instances are different.

@giovp
Copy link
Member Author

giovp commented Jul 8, 2024

@melonora here I would also need your green light since, due to the change of get_element_instances, the results of the tests had to be modified. In short, because now the get_element_instances doesn't return the background as default, hence the joins with the table produce a different results. Please see inline comments above, let me know if anything is unclear and if you agree with this.
I guess one option would be to expose the return_background in join, but I really don't see it to be of any value at the moment. Wdyt?

@giovp giovp requested a review from melonora July 8, 2024 14:02
@codecov
Copy link

codecov bot commented Jul 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.91%. Comparing base (95d69ff) to head (3b7026e).
Report is 45 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #621      +/-   ##
==========================================
- Coverage   91.93%   91.91%   -0.03%     
==========================================
  Files          44       44              
  Lines        6661     6666       +5     
==========================================
+ Hits         6124     6127       +3     
- Misses        537      539       +2     
Files with missing lines Coverage Δ
src/spatialdata/_core/centroids.py 100.00% <100.00%> (ø)
src/spatialdata/_core/query/relational_query.py 90.65% <100.00%> (-0.17%) ⬇️

... and 2 files with indirect coverage changes

Copy link
Collaborator

@melonora melonora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @giovp, LGTM! However as you said I agree that it does not make sense to have it in the join, but I would add it in the extra explanation of the docstring for the function before merging, but for now I pre-approve. Did you already check whether any tests on the other repos fail?

@giovp
Copy link
Member Author

giovp commented Jul 9, 2024

@melonora indeed some tests in spatialdata-plot fails, so I will open a separate PR before merging this

@giovp giovp merged commit 5a96ca4 into main Jul 9, 2024
@giovp giovp deleted the giovp/get_centroids_label branch July 9, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

get_element_instances and get_centroids should not (?) return zero element

3 participants

Comments