-
Notifications
You must be signed in to change notification settings - Fork 17
coloring shapes by categorical variable #153
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
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #153 +/- ##
==========================================
+ Coverage 74.88% 78.98% +4.10%
==========================================
Files 11 11
Lines 1298 1342 +44
==========================================
+ Hits 972 1060 +88
+ Misses 326 282 -44
|
Is this bug already solved? Or were these diagnostic plots? |
Bug is solved, I still want to add some tests |
palette = ( | ||
ListedColormap(dict.fromkeys(color_vector)) if render_params.palette is None else render_params.palette | ||
) | ||
# remove the color of NaN values, else it might be assigned to a category |
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.
Better to have this comment directly next to the dict.from_keys
|
||
color_map = dict(zip(categories, _get_colors_for_categorical_obs(categories))) | ||
color_map = dict(zip(categories, _get_colors_for_categorical_obs(categories, palette))) | ||
# color_map = _get_palette( | ||
# adata=adata, cluster_key=value_to_plot, categories=categories, palette=palette, alpha=alpha |
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.
remove comment plz
tests/pl/test_render_shapes.py
Outdated
) | ||
|
||
sdata_blobs.pl.render_shapes( | ||
"blobs_polygons", color="cluster", groups=["c2", "c1"], palette=ListedColormap(["green", "yellow"]) |
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.
Please take care of the ListedColormap conversion inside the code, it's unintuitive to have the user feed it in.
CHANGELOG.md
Outdated
@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning][]. | |||
- Multipolygons are now handled correctly (#93) | |||
- Legend order is now deterministic (#143) | |||
- Images no longer normalised by default (#150) | |||
- Shapes can be colored by categorical variable with the `group` and `palette` arguments (#153) |
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.
Please make clear that groups
is used to filter the data and palette
is used to color the (potentially remaining) categories. Also, what happens if I add a palette argument to a continous variable?
Meeting notes @Sonja-Stockhaus
|
src/spatialdata_plot/pl/basic.py
Outdated
@@ -183,8 +183,11 @@ def render_shapes( | |||
Key in :attr:`anndata.AnnData.layers` or `None` for :attr:`anndata.AnnData.X`. | |||
palette | |||
Palette for discrete annotations, see :class:`matplotlib.colors.Colormap`. |
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.
L185 and L186 disagree, no?
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.
because of the :class:matplotlib.colors.Colormap
?
tests/pl/test_render_points.py
Outdated
@@ -21,3 +21,12 @@ | |||
class TestPoints(PlotTester, metaclass=PlotTesterMeta): | |||
def test_plot_can_render_points(self, sdata_blobs: SpatialData): | |||
sdata_blobs.pl.render_points(elements="blobs_points").pl.show() | |||
|
|||
def test_plot_can_filter_with_groups(self, sdata_blobs: SpatialData): | |||
sdata_blobs.pl.render_points(color="genes", groups="b", palette=["orange"]).pl.show() |
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.
Please change the palette
here to be just a string so we're also testing that minor functionality here
src/spatialdata_plot/pl/utils.py
Outdated
cmap = plt.get_cmap(palette) | ||
palette = [to_hex(x) for x in cmap(color_idx, alpha=alpha)] | ||
# cmap = plt.get_cmap(palette) | ||
# palette = [to_hex(x) for x in cmap(color_idx, alpha=alpha)] |
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.
remove comments
@@ -182,9 +182,13 @@ def render_shapes( | |||
layer | |||
Key in :attr:`anndata.AnnData.layers` or `None` for :attr:`anndata.AnnData.X`. | |||
palette | |||
Palette for discrete annotations, see :class:`matplotlib.colors.Colormap`. | |||
Palette for discrete annotations. List of valid color names that should be used | |||
for the categories (all or as specified by `groups`). For a single category, |
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.
good descriptions 👌
This should fix multiple problems when coloring shapes, namely:
1 Incorrect legend coloring when using the groups argument:
2 palette parameter in render_shapes doesn't have influence on the plot (= #145)
Plus some downstream bugfixes that came up after that