-
Notifications
You must be signed in to change notification settings - Fork 17
Refactor of colorbar
and norm
logic
#346
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
Refactor of colorbar
and norm
logic
#346
Conversation
for more information, see https://pre-commit.ci
…ears-to-be-broken-labels-associated-with-wrong-colors' of github.com:scverse/spatialdata-plot into bugfix/342-coloring-labels-by-a-continuous-variable-appears-to-be-broken-labels-associated-with-wrong-colors
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #346 +/- ##
==========================================
- Coverage 84.27% 83.76% -0.51%
==========================================
Files 8 8
Lines 1558 1540 -18
==========================================
- Hits 1313 1290 -23
- Misses 245 250 +5
|
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.
Seems good to me, though please check my comment also in #344 regarding the skimage issue. Also please add a PR description before merging with also the reasoning of getting rid of percentiles_for_norm:)
Co-authored-by: Wouter-Michiel Vierdag <[email protected]>
Apologies for chiming in here again, but I'm curious about the decision to remove sdata.pl.render_images(
'78_image',
channel=["CD45", "PanCK", "DAPI"]
).pl.show(coordinate_systems='78') Because of data like that, I've routinely started plotting like this: sdata.pl.render_images(
'78_image',
percentiles_for_norm=(0, 100),
channel=["CD45", "PanCK", "DAPI"]
).pl.show(coordinate_systems='78') How would you achieve something like this using just the |
Hm, interesting. Does passing a matplotlib |
One caveat that I see in that, this way, all channels are normalised based on the same vmin/vmax which might not necessarily work if your channels have different value ranges. We have a mostly undocumented Is the goal to scale all channels to [0,1]? I'd be happy to build out that parameter for this purpose |
yeah exactly, I think having some control over which channels get which vmin/vmax would be super helpful. Even with |
Unified
norm
logic across the 4 functions. Before, therender_images
function would take a separatepercentiles_for_norm
argument (legacy reasons) which would/could interfere with whatever was passed tonorm
. As a consequence of this, I fixed some of the other resulting images.