add argument `return_background` for `get_centroids` and `get_element_instances` in the case of labels by giovp · Pull Request #621 · scverse/spatialdata

giovp · 2024-07-08T09:10:30Z

closes#614

giovp · 2024-07-08T13:58:44Z

 )
- assert all(table.obs["instance_id"] == range(100))
+
+ assert all(table.obs["instance_id"] == range(1, 100))


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 · 2024-07-08T14:02:00Z

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

codecov · 2024-07-08T14:05:45Z

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

melonora

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 · 2024-07-09T07:58:26Z

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

giovp · 2024-07-08T09:10:30Z

closes#614

giovp · 2024-07-08T13:58:44Z

 )
- assert all(table.obs["instance_id"] == range(100))
+
+ assert all(table.obs["instance_id"] == range(1, 100))


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 · 2024-07-08T14:02:00Z

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

codecov · 2024-07-08T14:05:45Z

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

melonora

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 · 2024-07-09T07:58:26Z

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

giovp · 2024-07-08T09:10:30Z

closes#614

giovp · 2024-07-08T13:58:44Z

 )
- assert all(table.obs["instance_id"] == range(100))
+
+ assert all(table.obs["instance_id"] == range(1, 100))


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 · 2024-07-08T14:02:00Z

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

codecov · 2024-07-08T14:05:45Z

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

melonora

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 · 2024-07-09T07:58:26Z

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

giovp · 2024-07-08T09:10:30Z

closes#614

giovp · 2024-07-08T13:58:44Z

 )
- assert all(table.obs["instance_id"] == range(100))
+
+ assert all(table.obs["instance_id"] == range(1, 100))


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 · 2024-07-08T14:02:00Z

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

codecov · 2024-07-08T14:05:45Z

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

melonora

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 · 2024-07-09T07:58:26Z

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

giovp · 2024-07-08T09:10:30Z

closes#614

giovp · 2024-07-08T13:58:44Z

 )
- assert all(table.obs["instance_id"] == range(100))
+
+ assert all(table.obs["instance_id"] == range(1, 100))


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 · 2024-07-08T14:02:00Z

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

codecov · 2024-07-08T14:05:45Z

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

melonora

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 · 2024-07-09T07:58:26Z

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

giovp · 2024-07-08T09:10:30Z

closes#614

giovp · 2024-07-08T13:58:44Z

 )
- assert all(table.obs["instance_id"] == range(100))
+
+ assert all(table.obs["instance_id"] == range(1, 100))


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 · 2024-07-08T14:02:00Z

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

codecov · 2024-07-08T14:05:45Z

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

melonora

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 · 2024-07-09T07:58:26Z

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

giovp · 2024-07-08T09:10:30Z

closes#614

giovp · 2024-07-08T13:58:44Z

 )
- assert all(table.obs["instance_id"] == range(100))
+
+ assert all(table.obs["instance_id"] == range(1, 100))


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 · 2024-07-08T14:02:00Z

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

codecov · 2024-07-08T14:05:45Z

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

melonora

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 · 2024-07-09T07:58:26Z

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

giovp · 2024-07-08T09:10:30Z

closes#614

giovp · 2024-07-08T13:58:44Z

 )
- assert all(table.obs["instance_id"] == range(100))
+
+ assert all(table.obs["instance_id"] == range(1, 100))


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 · 2024-07-08T14:02:00Z

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

codecov · 2024-07-08T14:05:45Z

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

melonora

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 · 2024-07-09T07:58:26Z

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

add argument for and
45f1195

giovp requested a review from LucaMarconato July 8, 2024 09:10

LucaMarconato approved these changes Jul 8, 2024
View reviewed changes

fix join tests
c539c56

giovp commented Jul 8, 2024
View reviewed changes

cleanup
89e62eb

giovp requested a review from melonora July 8, 2024 14:02

melonora approved these changes Jul 8, 2024
View reviewed changes

add docs
69aaf74

giovp mentioned this pull request Jul 9, 2024
fix tests from modified get_element_instancesscverse/spatialdata-plot#287
Merged

giovp added 3 commits July 9, 2024 13:56

add note regardign the background label
f2a65df

try fixing docs
b531255

add release note
3b7026e

giovp merged commit 5a96ca4 into mainJul 9, 2024

giovp deleted the giovp/get_centroids_label branch July 9, 2024 14:18

add argument for and
45f1195

giovp requested a review from LucaMarconato July 8, 2024 09:10

LucaMarconato approved these changes Jul 8, 2024
View reviewed changes

fix join tests
c539c56

giovp commented Jul 8, 2024
View reviewed changes

cleanup
89e62eb

giovp requested a review from melonora July 8, 2024 14:02

melonora approved these changes Jul 8, 2024
View reviewed changes

add docs
69aaf74

giovp mentioned this pull request Jul 9, 2024
fix tests from modified get_element_instancesscverse/spatialdata-plot#287
Merged

giovp added 3 commits July 9, 2024 13:56

add note regardign the background label
f2a65df

try fixing docs
b531255

add release note
3b7026e

giovp merged commit 5a96ca4 into mainJul 9, 2024

giovp deleted the giovp/get_centroids_label branch July 9, 2024 14:18

add argument for and
45f1195

giovp requested a review from LucaMarconato July 8, 2024 09:10

LucaMarconato approved these changes Jul 8, 2024
View reviewed changes

fix join tests
c539c56

giovp commented Jul 8, 2024
View reviewed changes

cleanup
89e62eb

giovp requested a review from melonora July 8, 2024 14:02

melonora approved these changes Jul 8, 2024
View reviewed changes

add docs
69aaf74

giovp mentioned this pull request Jul 9, 2024
fix tests from modified get_element_instancesscverse/spatialdata-plot#287
Merged

giovp added 3 commits July 9, 2024 13:56

add note regardign the background label
f2a65df

try fixing docs
b531255

add release note
3b7026e

giovp merged commit 5a96ca4 into mainJul 9, 2024

giovp deleted the giovp/get_centroids_label branch July 9, 2024 14:18

add argument for and
45f1195

giovp requested a review from LucaMarconato July 8, 2024 09:10

LucaMarconato approved these changes Jul 8, 2024
View reviewed changes

fix join tests
c539c56

giovp commented Jul 8, 2024
View reviewed changes

cleanup
89e62eb

giovp requested a review from melonora July 8, 2024 14:02

melonora approved these changes Jul 8, 2024
View reviewed changes

add docs
69aaf74

giovp mentioned this pull request Jul 9, 2024
fix tests from modified get_element_instancesscverse/spatialdata-plot#287
Merged

giovp added 3 commits July 9, 2024 13:56

add note regardign the background label
f2a65df

try fixing docs
b531255

add release note
3b7026e

giovp merged commit 5a96ca4 into mainJul 9, 2024

giovp deleted the giovp/get_centroids_label branch July 9, 2024 14:18

add argument for and
45f1195

giovp requested a review from LucaMarconato July 8, 2024 09:10

LucaMarconato approved these changes Jul 8, 2024
View reviewed changes

fix join tests
c539c56

giovp commented Jul 8, 2024
View reviewed changes

cleanup
89e62eb

giovp requested a review from melonora July 8, 2024 14:02

melonora approved these changes Jul 8, 2024
View reviewed changes

add docs
69aaf74

giovp mentioned this pull request Jul 9, 2024
fix tests from modified get_element_instancesscverse/spatialdata-plot#287
Merged

giovp added 3 commits July 9, 2024 13:56

add note regardign the background label
f2a65df

try fixing docs
b531255

add release note
3b7026e

giovp merged commit 5a96ca4 into mainJul 9, 2024

giovp deleted the giovp/get_centroids_label branch July 9, 2024 14:18

add argument for and
45f1195

giovp requested a review from LucaMarconato July 8, 2024 09:10

LucaMarconato approved these changes Jul 8, 2024
View reviewed changes

fix join tests
c539c56

giovp commented Jul 8, 2024
View reviewed changes

cleanup
89e62eb

giovp requested a review from melonora July 8, 2024 14:02

melonora approved these changes Jul 8, 2024
View reviewed changes

add docs
69aaf74

giovp mentioned this pull request Jul 9, 2024
fix tests from modified get_element_instancesscverse/spatialdata-plot#287
Merged

giovp added 3 commits July 9, 2024 13:56

add note regardign the background label
f2a65df

try fixing docs
b531255

add release note
3b7026e

giovp merged commit 5a96ca4 into mainJul 9, 2024

giovp deleted the giovp/get_centroids_label branch July 9, 2024 14:18

add argument for and
45f1195

giovp requested a review from LucaMarconato July 8, 2024 09:10

LucaMarconato approved these changes Jul 8, 2024
View reviewed changes

fix join tests
c539c56

giovp commented Jul 8, 2024
View reviewed changes

cleanup
89e62eb

giovp requested a review from melonora July 8, 2024 14:02

melonora approved these changes Jul 8, 2024
View reviewed changes

add docs
69aaf74

giovp mentioned this pull request Jul 9, 2024
fix tests from modified get_element_instancesscverse/spatialdata-plot#287
Merged

giovp added 3 commits July 9, 2024 13:56

add note regardign the background label
f2a65df

try fixing docs
b531255

add release note
3b7026e

giovp merged commit 5a96ca4 into mainJul 9, 2024

giovp deleted the giovp/get_centroids_label branch July 9, 2024 14:18

add argument for and
45f1195

giovp requested a review from LucaMarconato July 8, 2024 09:10

LucaMarconato approved these changes Jul 8, 2024
View reviewed changes

fix join tests
c539c56

giovp commented Jul 8, 2024
View reviewed changes

cleanup
89e62eb

giovp requested a review from melonora July 8, 2024 14:02

melonora approved these changes Jul 8, 2024
View reviewed changes

add docs
69aaf74

giovp mentioned this pull request Jul 9, 2024
fix tests from modified get_element_instancesscverse/spatialdata-plot#287
Merged

giovp added 3 commits July 9, 2024 13:56

add note regardign the background label
f2a65df

try fixing docs
b531255

add release note
3b7026e

giovp merged commit 5a96ca4 into mainJul 9, 2024

giovp deleted the giovp/get_centroids_label branch July 9, 2024 14:18

Uh oh!

Conversation

giovp commented Jul 8, 2024

Uh oh!

giovpJul 8, 2024

Choose a reason for hiding this comment

Uh oh!

giovp commented Jul 8, 2024

Uh oh!

codecovBot commented Jul 8, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

melonora left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giovp commented Jul 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

giovp commented Jul 8, 2024

Uh oh!

giovpJul 8, 2024

Choose a reason for hiding this comment

Uh oh!

giovp commented Jul 8, 2024

Uh oh!

codecovBot commented Jul 8, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

melonora left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giovp commented Jul 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

giovp commented Jul 8, 2024

Uh oh!

giovpJul 8, 2024

Choose a reason for hiding this comment

Uh oh!

giovp commented Jul 8, 2024

Uh oh!

codecovBot commented Jul 8, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

melonora left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giovp commented Jul 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecovBot commented Jul 8, 2024•
edited
Loading

melonora left a comment •
edited
Loading

codecovBot commented Jul 8, 2024•
edited
Loading

melonora left a comment •
edited
Loading

codecovBot commented Jul 8, 2024•
edited
Loading

melonora left a comment •
edited
Loading

codecovBot commented Jul 8, 2024•
edited
Loading

melonora left a comment •
edited
Loading

codecovBot commented Jul 8, 2024•
edited
Loading

melonora left a comment •
edited
Loading