Fix legend of colour-mapped scatter plots. #19816
Conversation
lib/matplotlib/collections.py
Outdated
| # Allow possibility to call 'self.set_array(None)'. | ||
| if self._check_update("array") and self._A is not None: | ||
| if self._A is not None: |
There was a problem hiding this comment.
The issue is that this was coming back false, but we should have gone through this branch?
lib/matplotlib/collections.py
Outdated
| # Allow possibility to call 'self.set_array(None)'. | ||
| if self._check_update("array") and self._A is not None: | ||
| if self._A is not None: |
There was a problem hiding this comment.
Does this always return True? Otherwise, self._mapped_colors will not be calculated here, and has the default value None. In the following lines
matplotlib/lib/matplotlib/collections.py
Lines 927 to 934 in e6fd901
the _facecolors and _edgecolors may be set as self._mapped_colors, which is None. Then cause trouble in the evaluation in draw:
matplotlib/lib/matplotlib/collections.py
Lines 379 to 380 in e6fd901
So I suggest to modify Lines 931 - 935 to
if self._face_is_mapped and self._mapped_colors is not None:
self._facecolors = self._mapped_colors
else:
self._set_facecolor(self._original_facecolor)
if self._edge_is_mapped and self._mapped_colors is not None:
self._edgecolors = self._mapped_colorsThere was a problem hiding this comment.
Actually, line 913 in the green could be replaced with
if self._face_is_mapped or self._edge_is_mapped:
because the check for self._A is not None is being done in the _set_mappable_flags helper immediately before this.
This change would make the logic clearer.
There is no need in any case for the suggested modification of lines 931-935.
With the loss of the check_update call, the efficiency I was trying to achieve--recalculating the mapping only if something changed--is gone, so I think there is no longer a need to have the self._mapped_colors variable. It can be local to the function, because it is always being recalculated if it is going to be used..
There was a problem hiding this comment.
This is OK, but in my comment I recommended changing the one critical line to more clearly express the current logic. Additional cleanup (removal of the private attribute, assuming we don't add logic to figure out whether the mapping changed) can be deferred to master or done here.
|
So, it was bugging me a bit why this was triggered, so I spent a bit of time looking closer. The problem stems from matplotlib/lib/matplotlib/collections.py Lines 942 to 962 in 315ff06 but as the comment wonders, does not copy _update_dict, nor does it copy the cached values that were added in #18480. This is how we somehow have an array, but no mapped colours.
So while this PR fixes it, basically by forcing an update, I'm not sure it's a complete fix. I think instead, we may want to fix |
|
At the very least, I expect we'll want to do: # update_from for scalarmappable
self._A = other._A
self.norm = other.norm
self.cmap = other.cmap
- # do we need to copy self._update_dict? -JJL
+ self._update_dict = other._update_dict.copy()
self.stale = Trueand probably also: artist.Artist.update_from(self, other)
self._antialiaseds = other._antialiaseds
+ self._mapped_colors = other._mapped_colors
+ self._edge_is_mapped = other._edge_is_mapped
self._original_edgecolor = other._original_edgecolor
self._edgecolors = other._edgecolors
+ self._face_is_mapped = other._face_is_mapped
self._original_facecolor = other._original_facecolor
self._facecolors = other._facecolors
self._linewidths = other._linewidthsbut there are several others that might be missing. Maybe I should defer those till later until we can check the scope of their effect? |
These were added in matplotlib#18480, and not copying them can mean that an artist that used `update_from` could get in an inconsistent state. For example, this will fix legends of colour-mapped scatter plots, where a copy of the scatter Collection is made to go in the legend. Fixes matplotlib#19779.
7c1b2c0 to
ee8dc54
Compare
|
OK, this now does as I said, changing |
|
Presumably we want the same fix on the default branch and will handle that via the merge up? |
|
It's fixed there because the |
|
I'm going to go ahead and merge this on one just my review in the interest of time so we can get 3.4.1 out. |
|
Thank you, @QuLogic, it was good that you took a closer look. |
PR Summary
This is a slight backport of ed6d92b, namely ignoring the array update status when updating the scalar mappable.
To not break the watchers though, the check must still be called to update the status.
Fixes #19779.
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).