-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fix clims when plotting shapes element annotations with matplotlib rendering #368
base: main
Are you sure you want to change the base?
Changes from 5 commits
0744fc4
17b9753
31cfc57
bff962e
a89a73e
440100a
fcf1549
e652ba2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,6 @@ | |
LinearSegmentedColormap, | ||
ListedColormap, | ||
Normalize, | ||
TwoSlopeNorm, | ||
to_rgba, | ||
) | ||
from matplotlib.figure import Figure | ||
|
@@ -339,7 +338,7 @@ def _get_collection_shape( | |
c = cmap(c) | ||
else: | ||
try: | ||
norm = colors.Normalize(vmin=min(c), vmax=max(c)) | ||
norm = colors.Normalize(vmin=min(c), vmax=max(c)) if norm is None else norm | ||
except ValueError as e: | ||
raise ValueError( | ||
"Could not convert values in the `color` column to float, if `color` column represents" | ||
|
@@ -353,7 +352,7 @@ def _get_collection_shape( | |
c = cmap(c) | ||
else: | ||
try: | ||
norm = colors.Normalize(vmin=min(c), vmax=max(c)) | ||
norm = colors.Normalize(vmin=min(c), vmax=max(c)) if norm is None else norm | ||
except ValueError as e: | ||
raise ValueError( | ||
"Could not convert values in the `color` column to float, if `color` column represents" | ||
|
@@ -506,12 +505,6 @@ def _prepare_cmap_norm( | |
|
||
if norm is None: | ||
norm = Normalize(vmin=vmin, vmax=vmax, clip=True) | ||
elif isinstance(norm, Normalize) or not norm: | ||
pass # TODO | ||
elif vcenter is None: | ||
norm = Normalize(vmin=vmin, vmax=vmax, clip=True) | ||
else: | ||
norm = TwoSlopeNorm(vmin=vmin, vmax=vmax, vcenter=vcenter) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that |
||
|
||
na_color, na_color_modified_by_user = _sanitise_na_color(na_color) | ||
cmap.set_bad(na_color) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are the two files different? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just different color perhaps due to made changes and then generating the labels with a different color. Other than that there are no changes. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -144,7 +144,7 @@ def test_plot_colorbar_respects_input_limits(self, sdata_blobs: SpatialData): | |
sdata_blobs.pl.render_shapes("blobs_polygons", color="cluster", groups=["c1"]).pl.show() | ||
|
||
def test_plot_colorbar_can_be_normalised(self, sdata_blobs: SpatialData): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does
But I may be missing something and be wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it does nothing. @timtreis is this purely to test that the colorbar stays limited from 1-50? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed the argument now as typically it is used to just color certain categorical values within a column, which from the title of the test function definition is not the intention. |
||
sdata_blobs["table"].obs["region"] = ["blobs_polygons"] * sdata_blobs["table"].n_obs | ||
sdata_blobs["table"].obs["region"] = "blobs_polygons" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no region is the |
||
sdata_blobs["table"].uns["spatialdata_attrs"]["region"] = "blobs_polygons" | ||
sdata_blobs.shapes["blobs_polygons"]["cluster"] = [1, 2, 3, 5, 20] | ||
norm = Normalize(vmin=0, vmax=5, clip=True) | ||
|
@@ -186,7 +186,7 @@ def test_plot_can_plot_with_annotation_despite_random_shuffling(self, sdata_blob | |
|
||
def test_plot_can_plot_queried_with_annotation_despite_random_shuffling(self, sdata_blobs: SpatialData): | ||
sdata_blobs["table"].obs["region"] = "blobs_circles" | ||
new_table = sdata_blobs["table"][:5] | ||
new_table = sdata_blobs["table"][:5].copy() | ||
new_table.uns["spatialdata_attrs"]["region"] = "blobs_circles" | ||
new_table.obs["instance_id"] = np.array(range(5)) | ||
|
||
|
@@ -214,7 +214,7 @@ def test_plot_can_plot_queried_with_annotation_despite_random_shuffling(self, sd | |
|
||
def test_plot_can_color_two_shapes_elements_by_annotation(self, sdata_blobs: SpatialData): | ||
sdata_blobs["table"].obs["region"] = "blobs_circles" | ||
new_table = sdata_blobs["table"][:10] | ||
new_table = sdata_blobs["table"][:10].copy() | ||
new_table.uns["spatialdata_attrs"]["region"] = ["blobs_circles", "blobs_polygons"] | ||
new_table.obs["instance_id"] = np.concatenate((np.array(range(5)), np.array(range(5)))) | ||
|
||
|
@@ -230,7 +230,7 @@ def test_plot_can_color_two_shapes_elements_by_annotation(self, sdata_blobs: Spa | |
|
||
def test_plot_can_color_two_queried_shapes_elements_by_annotation(self, sdata_blobs: SpatialData): | ||
sdata_blobs["table"].obs["region"] = "blobs_circles" | ||
new_table = sdata_blobs["table"][:10] | ||
new_table = sdata_blobs["table"][:10].copy() | ||
new_table.uns["spatialdata_attrs"]["region"] = ["blobs_circles", "blobs_polygons"] | ||
new_table.obs["instance_id"] = np.concatenate((np.array(range(5)), np.array(range(5)))) | ||
|
||
|
@@ -316,3 +316,16 @@ def test_plot_datashader_can_color_by_value(self, sdata_blobs: SpatialData): | |
sdata_blobs["table"].uns["spatialdata_attrs"]["region"] = "blobs_polygons" | ||
sdata_blobs.shapes["blobs_polygons"]["value"] = [1, 10, 1, 20, 1] | ||
sdata_blobs.pl.render_shapes(element="blobs_polygons", color="value", method="datashader").pl.show() | ||
|
||
def test_plot_can_set_clims_clip(self, sdata_blobs: SpatialData): | ||
table_shapes = sdata_blobs["table"][:5].copy() | ||
table_shapes.obs.instance_id = list(range(5)) | ||
table_shapes.obs["region"] = "blobs_circles" | ||
table_shapes.obs["dummy_gene_expression"] = [i * 10 for i in range(5)] | ||
table_shapes.uns["spatialdata_attrs"]["region"] = "blobs_circles" | ||
sdata_blobs["new_table"] = table_shapes | ||
|
||
norm = Normalize(vmin=20, vmax=40, clip=True) | ||
sdata_blobs.pl.render_shapes( | ||
"blobs_circles", color="dummy_gene_expression", norm=norm, table_name="new_table" | ||
).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.
In this function
Normalize()
is initialized withoutclip
, while in_prepare_cmap_norm()
the default is to setclip=True
. I would choose one of the two as our default choice. The user will be able to specifyclip
,vcenter
, etc by passing anorm
object directly.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.
hmm let me double check that if we don't pass norm as user, whether ultimately the norm is always created anyway, then we can get rid of normalize instance initiated here.
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.
ok there is code left over of when vmin and vmax were removed. Not certain whether to address this in a different PR.