-
Notifications
You must be signed in to change notification settings - Fork 17
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
Conversation
basic shapes datashader rendering
@Sonja-Stockhaus recently we introduced datashader support in 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 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 |
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. |
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #244 +/- ##
=======================================
Coverage ? 80.19%
=======================================
Files ? 11
Lines ? 1737
Branches ? 0
=======================================
Hits ? 1393
Misses ? 344
Partials ? 0
|
@Sonja-Stockhaus is this ready for review? |
There was a problem hiding this 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?
src/spatialdata_plot/pl/render.py
Outdated
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)? |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
Work from @Sonja-Stockhaus and @ckmah.