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

Rasterize shapes #566

Merged
merged 27 commits into from
Jun 9, 2024
Merged

Rasterize shapes #566

merged 27 commits into from
Jun 9, 2024

Conversation

quentinblampey
Copy link
Contributor

@quentinblampey quentinblampey commented May 24, 2024

Shapes rasterization, i.e. converting shapes (e.g. polygons or points) into images

Use cases

values type Shapes or Points return_single_channel datashader reduction
None Point NA count
None Shapes True first
None Shapes False count_cat
category NA True first
category NA False count_cat
int / float NA NA sum

Speed test

Speed: 30s on 160k cells locally

Checklist

  • Add tests for both polygons (rectangles with MapAxis transform) and points
  • Add notebook tutorial, see spatialdata-sandbox
  • Add dict in attrs for the categories mapping

Copy link

codecov bot commented May 24, 2024

Codecov Report

Attention: Patch coverage is 85.90604% with 21 lines in your changes missing coverage. Please review.

Project coverage is 92.47%. Comparing base (62a6440) to head (9f8575f).
Report is 78 commits behind head on main.

Files with missing lines Patch % Lines
src/spatialdata/_core/operations/rasterize.py 85.71% 18 Missing ⚠️
src/spatialdata/_core/query/relational_query.py 77.77% 2 Missing ⚠️
src/spatialdata/_core/operations/_utils.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #566      +/-   ##
==========================================
- Coverage   92.56%   92.47%   -0.10%     
==========================================
  Files          42       42              
  Lines        6078     6177      +99     
==========================================
+ Hits         5626     5712      +86     
- Misses        452      465      +13     
Files with missing lines Coverage Δ
src/spatialdata/_core/operations/aggregate.py 94.38% <100.00%> (+0.32%) ⬆️
src/spatialdata/_utils.py 88.02% <100.00%> (+0.17%) ⬆️
src/spatialdata/_core/operations/_utils.py 92.30% <88.88%> (-0.72%) ⬇️
src/spatialdata/_core/query/relational_query.py 90.83% <77.77%> (-4.68%) ⬇️
src/spatialdata/_core/operations/rasterize.py 90.52% <85.71%> (+6.58%) ⬆️

... and 1 file with indirect coverage changes

@LucaMarconato
Copy link
Member

LucaMarconato commented May 29, 2024

Quick comments from discussion. Please add the following:

  • the table of this PR in the docs. Also:
  • I would explain that count (first row) acts like having a column of ones;
  • SKIPPED count_cat could be paired with a text example

Comments on the code

  • please add the test for points
  • test for an anndata object annotating the shapes/points

@LucaMarconato
Copy link
Member

LucaMarconato commented May 29, 2024

Luca's task before merging:

  • implement the single dispatch against SpatialData objects
  • labels should also accept table annotations
  • testing the rasterization of shapes and points into labels when the index of the element has not int dtype but str
  • test when rasterizing an entire SpatialData object

Tasks for a follow up PR:

  • extend value_key to accept a list of strings, as we do in aggregate()
  • implement the missing cases of aggregate() that now will defer the work to rasterize()
  • use rasterize() in spatialdata-plot instead of reimplementing the usage of datashader

@quentinblampey
Copy link
Contributor Author

I removed instance_key_as_default_value_key, and updated the above table of use case. What do you think of the new table @LucaMarconato?

@LucaMarconato
Copy link
Member

Thanks looks great! I am working on the tasks listed in my two comments; I will tag you for a review of that part when I finish, thanks 😊

@LucaMarconato
Copy link
Member

Luca's task before merging:

  • implement the single dispatch against SpatialData objects
  • labels should also accept table annotations
  • testing the rasterization of shapes and points into labels when the index of the element has not int dtype but str
  • test when rasterizing an entire SpatialData object

@quentinblampey I have just finalized the implementation. Could you please give a last round of review to the new changes? Thanks!

Copy link
Contributor Author

@quentinblampey quentinblampey left a comment

Choose a reason for hiding this comment

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

Looks good to me!
One small question: are we necessarily supposed to provide element_name if we want to get values from the table? Because I don't see how we can get the values if element_name is None (according to the current state of the code), which is not very convenient
(I actually have another small question but I can ask you tomorrow)

@LucaMarconato
Copy link
Member

LucaMarconato commented Jun 9, 2024

Thanks for checking the code. Actually what you spotted is an idiosyncrasy in the APIs: the public API rasterize() does not contain the element_name parameter, since this is specified in data when sdata is None, while the internal APIs to rasterized images/labels and points/shapes always have data as a SpatialElement and, if provided element_name as str.

Therefore, if we want a value from a table, sdata must be specified and data must be the element name.

I am honestly not super convinced of the API, but when we did a poll with users and other devs, they found it handier, so we decide to favor that implementation.

@LucaMarconato
Copy link
Member

(I actually have another small question but I can ask you tomorrow)

Ok nice, then gonna merge now 😊

@LucaMarconato LucaMarconato enabled auto-merge (squash) June 9, 2024 16:57
@LucaMarconato LucaMarconato merged commit 592561f into scverse:main Jun 9, 2024
8 checks passed
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.

2 participants