Refactor of colorbar and norm logic

[timtreis]

Unified norm logic across the 4 functions. Before, the render_images function would take a separate percentiles_for_norm argument (legacy reasons) which would/could interfere with whatever was passed to norm. As a consequence of this, I fixed some of the other resulting images.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.76%. Comparing base (393315b) to head (667c689).
Report is 2 commits behind head on main.

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 

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/basic.py	90.86% <ø> (ø)
src/spatialdata_plot/pl/render.py	94.72% <100.00%> (+0.31%)	⬆️
src/spatialdata_plot/pl/utils.py	76.16% <ø> (-1.13%)	⬇️

[melonora]

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:)

[clwgg]

Apologies for chiming in here again, but I'm curious about the decision to remove percentiles_for_norm, and I think it would be helpful to get some pointers on how its functionality should be replaced once I pull in these changes. I've found its capacity to do per-channel normalization quite helpful when there are extreme differences in the intensities of different markers. For example, using this:

sdata.pl.render_images(
'78_image',
channel=["CD45", "PanCK", "DAPI"]
).pl.show(coordinate_systems='78')

I'd get an image like this:

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')

which for this example gives:

How would you achieve something like this using just the norm flag?

[timtreis]

Hm, interesting. Does passing a matplotlib Normalise achieve a different outcome? I've also modified the norm parameter to no longer be a flag but instead sth that you can pass a matplotlib.colors.Normalise object.

[timtreis]

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 transfunc parameter in one of the functions (currently on the go, will check later).

Is the goal to scale all channels to [0,1]? I'd be happy to build out that parameter for this purpose

[clwgg]

yeah exactly, I think having some control over which channels get which vmin/vmax would be super helpful. Even with percentiles_for_norm I've sometimes wondered how it would look if I could set a list of (pmin, pmax) tuples with the same length as channel, to have some more fine-grained control over which channels to highlight. I suppose something similar could be achieved if it was possible to set a list of Normalize objects with the same length as channel. But achieving the task of percentiles_for_norm(0, 100) (which I think would still be the most common use-case for me) would get a lot more wordy since I'd first have to figure out the per-channel vmin/vmax corresponding to each channels min and max, and instantiate the appropriate Normalize objects to hand off to render_images.

[timtreis]

Unified norm logic across the 4 functions. Before, the render_images function would take a separate percentiles_for_norm argument (legacy reasons) which would/could interfere with whatever was passed to norm. As a consequence of this, I fixed some of the other resulting images.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.76%. Comparing base (393315b) to head (667c689).
Report is 2 commits behind head on main.

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 

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/basic.py	90.86% <ø> (ø)
src/spatialdata_plot/pl/render.py	94.72% <100.00%> (+0.31%)	⬆️
src/spatialdata_plot/pl/utils.py	76.16% <ø> (-1.13%)	⬇️

[melonora]

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:)

[clwgg]

Apologies for chiming in here again, but I'm curious about the decision to remove percentiles_for_norm, and I think it would be helpful to get some pointers on how its functionality should be replaced once I pull in these changes. I've found its capacity to do per-channel normalization quite helpful when there are extreme differences in the intensities of different markers. For example, using this:

sdata.pl.render_images(
'78_image',
channel=["CD45", "PanCK", "DAPI"]
).pl.show(coordinate_systems='78')

I'd get an image like this:

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')

which for this example gives:

How would you achieve something like this using just the norm flag?

[timtreis]

Hm, interesting. Does passing a matplotlib Normalise achieve a different outcome? I've also modified the norm parameter to no longer be a flag but instead sth that you can pass a matplotlib.colors.Normalise object.

[timtreis]

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 transfunc parameter in one of the functions (currently on the go, will check later).

Is the goal to scale all channels to [0,1]? I'd be happy to build out that parameter for this purpose

[clwgg]

yeah exactly, I think having some control over which channels get which vmin/vmax would be super helpful. Even with percentiles_for_norm I've sometimes wondered how it would look if I could set a list of (pmin, pmax) tuples with the same length as channel, to have some more fine-grained control over which channels to highlight. I suppose something similar could be achieved if it was possible to set a list of Normalize objects with the same length as channel. But achieving the task of percentiles_for_norm(0, 100) (which I think would still be the most common use-case for me) would get a lot more wordy since I'd first have to figure out the per-channel vmin/vmax corresponding to each channels min and max, and instantiate the appropriate Normalize objects to hand off to render_images.

[timtreis]

Unified norm logic across the 4 functions. Before, the render_images function would take a separate percentiles_for_norm argument (legacy reasons) which would/could interfere with whatever was passed to norm. As a consequence of this, I fixed some of the other resulting images.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.76%. Comparing base (393315b) to head (667c689).
Report is 2 commits behind head on main.

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 

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/basic.py	90.86% <ø> (ø)
src/spatialdata_plot/pl/render.py	94.72% <100.00%> (+0.31%)	⬆️
src/spatialdata_plot/pl/utils.py	76.16% <ø> (-1.13%)	⬇️

[melonora]

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:)

[clwgg]

Apologies for chiming in here again, but I'm curious about the decision to remove percentiles_for_norm, and I think it would be helpful to get some pointers on how its functionality should be replaced once I pull in these changes. I've found its capacity to do per-channel normalization quite helpful when there are extreme differences in the intensities of different markers. For example, using this:

sdata.pl.render_images(
'78_image',
channel=["CD45", "PanCK", "DAPI"]
).pl.show(coordinate_systems='78')

I'd get an image like this:

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')

which for this example gives:

How would you achieve something like this using just the norm flag?

[timtreis]

Hm, interesting. Does passing a matplotlib Normalise achieve a different outcome? I've also modified the norm parameter to no longer be a flag but instead sth that you can pass a matplotlib.colors.Normalise object.

[timtreis]

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 transfunc parameter in one of the functions (currently on the go, will check later).

Is the goal to scale all channels to [0,1]? I'd be happy to build out that parameter for this purpose

[clwgg]

yeah exactly, I think having some control over which channels get which vmin/vmax would be super helpful. Even with percentiles_for_norm I've sometimes wondered how it would look if I could set a list of (pmin, pmax) tuples with the same length as channel, to have some more fine-grained control over which channels to highlight. I suppose something similar could be achieved if it was possible to set a list of Normalize objects with the same length as channel. But achieving the task of percentiles_for_norm(0, 100) (which I think would still be the most common use-case for me) would get a lot more wordy since I'd first have to figure out the per-channel vmin/vmax corresponding to each channels min and max, and instantiate the appropriate Normalize objects to hand off to render_images.

[timtreis]

Unified norm logic across the 4 functions. Before, the render_images function would take a separate percentiles_for_norm argument (legacy reasons) which would/could interfere with whatever was passed to norm. As a consequence of this, I fixed some of the other resulting images.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.76%. Comparing base (393315b) to head (667c689).
Report is 2 commits behind head on main.

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 

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/basic.py	90.86% <ø> (ø)
src/spatialdata_plot/pl/render.py	94.72% <100.00%> (+0.31%)	⬆️
src/spatialdata_plot/pl/utils.py	76.16% <ø> (-1.13%)	⬇️

[melonora]

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:)

[clwgg]

Apologies for chiming in here again, but I'm curious about the decision to remove percentiles_for_norm, and I think it would be helpful to get some pointers on how its functionality should be replaced once I pull in these changes. I've found its capacity to do per-channel normalization quite helpful when there are extreme differences in the intensities of different markers. For example, using this:

sdata.pl.render_images(
'78_image',
channel=["CD45", "PanCK", "DAPI"]
).pl.show(coordinate_systems='78')

I'd get an image like this:

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')

which for this example gives:

How would you achieve something like this using just the norm flag?

[timtreis]

Hm, interesting. Does passing a matplotlib Normalise achieve a different outcome? I've also modified the norm parameter to no longer be a flag but instead sth that you can pass a matplotlib.colors.Normalise object.

[timtreis]

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 transfunc parameter in one of the functions (currently on the go, will check later).

Is the goal to scale all channels to [0,1]? I'd be happy to build out that parameter for this purpose

[clwgg]

yeah exactly, I think having some control over which channels get which vmin/vmax would be super helpful. Even with percentiles_for_norm I've sometimes wondered how it would look if I could set a list of (pmin, pmax) tuples with the same length as channel, to have some more fine-grained control over which channels to highlight. I suppose something similar could be achieved if it was possible to set a list of Normalize objects with the same length as channel. But achieving the task of percentiles_for_norm(0, 100) (which I think would still be the most common use-case for me) would get a lot more wordy since I'd first have to figure out the per-channel vmin/vmax corresponding to each channels min and max, and instantiate the appropriate Normalize objects to hand off to render_images.

[timtreis]

Unified norm logic across the 4 functions. Before, the render_images function would take a separate percentiles_for_norm argument (legacy reasons) which would/could interfere with whatever was passed to norm. As a consequence of this, I fixed some of the other resulting images.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.76%. Comparing base (393315b) to head (667c689).
Report is 2 commits behind head on main.

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 

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/basic.py	90.86% <ø> (ø)
src/spatialdata_plot/pl/render.py	94.72% <100.00%> (+0.31%)	⬆️
src/spatialdata_plot/pl/utils.py	76.16% <ø> (-1.13%)	⬇️

[melonora]

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:)

[clwgg]

Apologies for chiming in here again, but I'm curious about the decision to remove percentiles_for_norm, and I think it would be helpful to get some pointers on how its functionality should be replaced once I pull in these changes. I've found its capacity to do per-channel normalization quite helpful when there are extreme differences in the intensities of different markers. For example, using this:

sdata.pl.render_images(
'78_image',
channel=["CD45", "PanCK", "DAPI"]
).pl.show(coordinate_systems='78')

I'd get an image like this:

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')

which for this example gives:

How would you achieve something like this using just the norm flag?

[timtreis]

Hm, interesting. Does passing a matplotlib Normalise achieve a different outcome? I've also modified the norm parameter to no longer be a flag but instead sth that you can pass a matplotlib.colors.Normalise object.

[timtreis]

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 transfunc parameter in one of the functions (currently on the go, will check later).

Is the goal to scale all channels to [0,1]? I'd be happy to build out that parameter for this purpose

[clwgg]

yeah exactly, I think having some control over which channels get which vmin/vmax would be super helpful. Even with percentiles_for_norm I've sometimes wondered how it would look if I could set a list of (pmin, pmax) tuples with the same length as channel, to have some more fine-grained control over which channels to highlight. I suppose something similar could be achieved if it was possible to set a list of Normalize objects with the same length as channel. But achieving the task of percentiles_for_norm(0, 100) (which I think would still be the most common use-case for me) would get a lot more wordy since I'd first have to figure out the per-channel vmin/vmax corresponding to each channels min and max, and instantiate the appropriate Normalize objects to hand off to render_images.

[timtreis]

Unified norm logic across the 4 functions. Before, the render_images function would take a separate percentiles_for_norm argument (legacy reasons) which would/could interfere with whatever was passed to norm. As a consequence of this, I fixed some of the other resulting images.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.76%. Comparing base (393315b) to head (667c689).
Report is 2 commits behind head on main.

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 

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/basic.py	90.86% <ø> (ø)
src/spatialdata_plot/pl/render.py	94.72% <100.00%> (+0.31%)	⬆️
src/spatialdata_plot/pl/utils.py	76.16% <ø> (-1.13%)	⬇️

[melonora]

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:)

[clwgg]

Apologies for chiming in here again, but I'm curious about the decision to remove percentiles_for_norm, and I think it would be helpful to get some pointers on how its functionality should be replaced once I pull in these changes. I've found its capacity to do per-channel normalization quite helpful when there are extreme differences in the intensities of different markers. For example, using this:

sdata.pl.render_images(
'78_image',
channel=["CD45", "PanCK", "DAPI"]
).pl.show(coordinate_systems='78')

I'd get an image like this:

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')

which for this example gives:

How would you achieve something like this using just the norm flag?

[timtreis]

Hm, interesting. Does passing a matplotlib Normalise achieve a different outcome? I've also modified the norm parameter to no longer be a flag but instead sth that you can pass a matplotlib.colors.Normalise object.

[timtreis]

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 transfunc parameter in one of the functions (currently on the go, will check later).

Is the goal to scale all channels to [0,1]? I'd be happy to build out that parameter for this purpose

[clwgg]

yeah exactly, I think having some control over which channels get which vmin/vmax would be super helpful. Even with percentiles_for_norm I've sometimes wondered how it would look if I could set a list of (pmin, pmax) tuples with the same length as channel, to have some more fine-grained control over which channels to highlight. I suppose something similar could be achieved if it was possible to set a list of Normalize objects with the same length as channel. But achieving the task of percentiles_for_norm(0, 100) (which I think would still be the most common use-case for me) would get a lot more wordy since I'd first have to figure out the per-channel vmin/vmax corresponding to each channels min and max, and instantiate the appropriate Normalize objects to hand off to render_images.

[timtreis]

Unified norm logic across the 4 functions. Before, the render_images function would take a separate percentiles_for_norm argument (legacy reasons) which would/could interfere with whatever was passed to norm. As a consequence of this, I fixed some of the other resulting images.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.76%. Comparing base (393315b) to head (667c689).
Report is 2 commits behind head on main.

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 

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/basic.py	90.86% <ø> (ø)
src/spatialdata_plot/pl/render.py	94.72% <100.00%> (+0.31%)	⬆️
src/spatialdata_plot/pl/utils.py	76.16% <ø> (-1.13%)	⬇️

[melonora]

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:)

[clwgg]

Apologies for chiming in here again, but I'm curious about the decision to remove percentiles_for_norm, and I think it would be helpful to get some pointers on how its functionality should be replaced once I pull in these changes. I've found its capacity to do per-channel normalization quite helpful when there are extreme differences in the intensities of different markers. For example, using this:

sdata.pl.render_images(
'78_image',
channel=["CD45", "PanCK", "DAPI"]
).pl.show(coordinate_systems='78')

I'd get an image like this:

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')

which for this example gives:

How would you achieve something like this using just the norm flag?

[timtreis]

Hm, interesting. Does passing a matplotlib Normalise achieve a different outcome? I've also modified the norm parameter to no longer be a flag but instead sth that you can pass a matplotlib.colors.Normalise object.

[timtreis]

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 transfunc parameter in one of the functions (currently on the go, will check later).

Is the goal to scale all channels to [0,1]? I'd be happy to build out that parameter for this purpose

[clwgg]

yeah exactly, I think having some control over which channels get which vmin/vmax would be super helpful. Even with percentiles_for_norm I've sometimes wondered how it would look if I could set a list of (pmin, pmax) tuples with the same length as channel, to have some more fine-grained control over which channels to highlight. I suppose something similar could be achieved if it was possible to set a list of Normalize objects with the same length as channel. But achieving the task of percentiles_for_norm(0, 100) (which I think would still be the most common use-case for me) would get a lot more wordy since I'd first have to figure out the per-channel vmin/vmax corresponding to each channels min and max, and instantiate the appropriate Normalize objects to hand off to render_images.

[timtreis]

Unified norm logic across the 4 functions. Before, the render_images function would take a separate percentiles_for_norm argument (legacy reasons) which would/could interfere with whatever was passed to norm. As a consequence of this, I fixed some of the other resulting images.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.76%. Comparing base (393315b) to head (667c689).
Report is 2 commits behind head on main.

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 

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/basic.py	90.86% <ø> (ø)
src/spatialdata_plot/pl/render.py	94.72% <100.00%> (+0.31%)	⬆️
src/spatialdata_plot/pl/utils.py	76.16% <ø> (-1.13%)	⬇️

[melonora]

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:)

[clwgg]

Apologies for chiming in here again, but I'm curious about the decision to remove percentiles_for_norm, and I think it would be helpful to get some pointers on how its functionality should be replaced once I pull in these changes. I've found its capacity to do per-channel normalization quite helpful when there are extreme differences in the intensities of different markers. For example, using this:

sdata.pl.render_images(
'78_image',
channel=["CD45", "PanCK", "DAPI"]
).pl.show(coordinate_systems='78')

I'd get an image like this:

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')

which for this example gives:

How would you achieve something like this using just the norm flag?

[timtreis]

Hm, interesting. Does passing a matplotlib Normalise achieve a different outcome? I've also modified the norm parameter to no longer be a flag but instead sth that you can pass a matplotlib.colors.Normalise object.

[timtreis]

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 transfunc parameter in one of the functions (currently on the go, will check later).

Is the goal to scale all channels to [0,1]? I'd be happy to build out that parameter for this purpose

[clwgg]

yeah exactly, I think having some control over which channels get which vmin/vmax would be super helpful. Even with percentiles_for_norm I've sometimes wondered how it would look if I could set a list of (pmin, pmax) tuples with the same length as channel, to have some more fine-grained control over which channels to highlight. I suppose something similar could be achieved if it was possible to set a list of Normalize objects with the same length as channel. But achieving the task of percentiles_for_norm(0, 100) (which I think would still be the most common use-case for me) would get a lot more wordy since I'd first have to figure out the per-channel vmin/vmax corresponding to each channels min and max, and instantiate the appropriate Normalize objects to hand off to render_images.