Fix clims when plotting shapes element annotations with matplotlib rendering #368
Conversation
melonora
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #368 +/- ##
==========================================
- Coverage 83.76% 83.73% -0.04%
==========================================
Files 8 8
Lines 1540 1543 +3
==========================================
+ Hits 1290 1292 +2
- Misses 250 251 +1
|
|
There were some tests that had an expected figure that was wrong in the first place so I fixed that. Think it is good to go for now. Here and there in the test code base I also saw no copies of anndata being created, spamming warnings so I silenced those. |
| 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.
Now that vcenter is removed, it should be removed also from the function signature. Also, kwargs is in the signature but not used, so I would remove it.
| @@ -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 | |||
There was a problem hiding this comment.
In this function Normalize() is initialized without clip, while in _prepare_cmap_norm() the default is to set clip=True. I would choose one of the two as our default choice. The user will be able to specify clip, vcenter, etc by passing a norm object directly.
There was a problem hiding this comment.
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.
ok there is code left over of when vmin and vmax were removed. Not certain whether to address this in a different PR.
There was a problem hiding this comment.
I'd address the choice of the value of clip in this PR please, because it's easy to forget about this in a new PR.
There was a problem hiding this comment.
default is set to False now
tests/_images/Labels_can_render_given_scale_of_multiscale_labels.png
Outdated
Show resolved
Hide resolved
| @@ -490,11 +490,8 @@ def _prepare_cmap_norm( | |||
| cmap: Colormap | str | None = None, | |||
| norm: Normalize | None = None, | |||
| na_color: ColorLike | None = None, | |||
| vmin: float | None = None, | |||
There was a problem hiding this comment.
@timtreis I don't remember the outcome of the discussion with the user that reported this. Is this the way to go (=letting users only use norm and not vmin, vmax) or shall we remove vcenter only and keep vmin, vmax and use them to initialize the default Normalize object?
There was a problem hiding this comment.
In the discussion it was stated that vmin and vmax are removed. This function is only internally called
There was a problem hiding this comment.
So we agree on clip 'True' by default if user does not provide normaloze object?
There was a problem hiding this comment.
If vmin and vmax are not exposed to the user (and hence they are None), then clip will have no effect because when exposed vmin, vmax are None, the data limits are used. So I would keep the default clip to be False (which is matplotlib's default).
There was a problem hiding this comment.
One thing, if vmin, vmax are removed from pl.render_shapes(), we should throw an informative exception or deprecation warning, explaining to the user that norm should be used instead. Could you add that please?
There was a problem hiding this comment.
They have not been removed in this PR though and the public functions thus already did not contain these parameters
There was a problem hiding this comment.
I did add a deprecation warning in case of the arguments being passed as kwargs
|
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
|
added todo as well to add this to tutorial notebook. |
closes #324
This PR fixes the issue described in #324. When creating the patch collection a default Normalize was always created instead of using the one provided by the user. Furthermore in this PR some legacy code was removed which did not do anything.