Skip to content

Datashader support for points and shapes #244

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

Merged
merged 26 commits into from
Jul 10, 2024

Conversation

LucaMarconato
Copy link
Member

Work from @Sonja-Stockhaus and @ckmah.

@LucaMarconato LucaMarconato linked an issue Apr 3, 2024 that may be closed by this pull request
@LucaMarconato
Copy link
Member Author

@Sonja-Stockhaus recently we introduced datashader support in spatialdata for points and shapes and you may find it useful to reuse some components.

In particular, you may find it useful to reuse the code in https://github.com/scverse/spatialdata/blob/b8fbc5c4ce7643455768a94665496ea4987294dd/src/spatialdata/_core/operations/rasterize.py#L604.

Technically, I would consider isolating the code used to compute cnv = ds.Canvas(...) into a private function of rasterize.py in spatialdata, and then I would use that private function from spatialdata-plot (using rasterize_shapes_points() directly may not be useful for spatialdata-plot since you probably want more control on the shading operation, so reusing some intermediate code by placing it on private functions may be the way to go).

Please let me know what you think about this. Also, if you prefer to merge this PR as it is and then eventually reuse the components from rasterize.py in a follow up refactoring, that would also be perfectly fine.

@Sonja-Stockhaus
Copy link
Collaborator

@Sonja-Stockhaus recently we introduced datashader support in spatialdata for points and shapes and you may find it useful to reuse some components.

In particular, you may find it useful to reuse the code in scverse/spatialdata@b8fbc5c/src/spatialdata/_core/operations/rasterize.py#L604.

Technically, I would consider isolating the code used to compute cnv = ds.Canvas(...) into a private function of rasterize.py in spatialdata, and then I would use that private function from spatialdata-plot (using rasterize_shapes_points() directly may not be useful for spatialdata-plot since you probably want more control on the shading operation, so reusing some intermediate code by placing it on private functions may be the way to go).

Please let me know what you think about this. Also, if you prefer to merge this PR as it is and then eventually reuse the components from rasterize.py in a follow up refactoring, that would also be perfectly fine.

Thanks for the heads up! I think fo now it would be best to finish and merge this PR and then do the refactoring later, since it's already so big.
Also regarding that @LucaMarconato can you tell me when the next pip release of spatialdata will be? Since this code is currently using some of the newer changes and that's why the tests keep failing right now

@LucaMarconato
Copy link
Member Author

Your message comes at the right time 😊 I just made a new release 10 minutes ago!

I agree, let's rather merge this and refactor later.

@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2024

Codecov Report

Attention: Patch coverage is 91.72932% with 11 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@80dfe4b). Learn more about missing BASE report.
Report is 11 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #244   +/-   ##
=======================================
  Coverage        ?   80.19%           
=======================================
  Files           ?       11           
  Lines           ?     1737           
  Branches        ?        0           
=======================================
  Hits            ?     1393           
  Misses          ?      344           
  Partials        ?        0           
Files Coverage Δ
src/spatialdata_plot/pl/render_params.py 100.00% <100.00%> (ø)
src/spatialdata_plot/pl/utils.py 69.76% <75.00%> (ø)
src/spatialdata_plot/pl/basic.py 90.90% <60.00%> (ø)
src/spatialdata_plot/pl/render.py 93.98% <93.22%> (ø)

@timtreis
Copy link
Member

timtreis commented Jul 9, 2024

@Sonja-Stockhaus is this ready for review?

@timtreis timtreis marked this pull request as ready for review July 9, 2024 21:14
@timtreis timtreis changed the title Datashader support Datashader support for points and shapes Jul 9, 2024
Copy link
Member

@timtreis timtreis left a comment

Choose a reason for hiding this comment

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

Already looks fairly good! Can we also add examples to your speed improvement notebook to show these?

cmap=color_vector[0][:-2],
color_key=color_key,
min_alpha=np.min([150, render_params.alpha * 255]),
) # TODO: choose other value than 150 for min_alpha (here and below)?
Copy link
Member

Choose a reason for hiding this comment

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

Why a different one? Is 150 bad?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not necessarily, but it's an arbitrary value that I chose to make everything better visible...

@Sonja-Stockhaus Sonja-Stockhaus merged commit 67a1699 into main Jul 10, 2024
5 checks passed
@Sonja-Stockhaus Sonja-Stockhaus deleted the feat/issue209_datashader branch July 10, 2024 19:29
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.

Use DataShader to speed up rendering of points Order of render commands doesn't reflect z-order of elements in plot
5 participants