Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix legend of colour-mapped scatter plots.#19816
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does this always return True? Otherwise,self._mapped_colors will not be calculated here, and has the default valueNone. In the following lines
matplotlib/lib/matplotlib/collections.py
Lines 927 to 934 ine6fd901
| ifself._face_is_mapped: | |
| self._facecolors=self._mapped_colors | |
| else: | |
| self._set_facecolor(self._original_facecolor) | |
| ifself._edge_is_mapped: | |
| self._edgecolors=self._mapped_colors | |
| else: | |
| self._set_edgecolor(self._original_edgecolor) |
the_facecolors and_edgecolors may be set asself._mapped_colors, which isNone. Then cause trouble in the evaluation indraw:
matplotlib/lib/matplotlib/collections.py
Lines 379 to 380 ine6fd901
| if (len(paths)==1andlen(trans)<=1and | |
| len(facecolors)==1andlen(edgecolors)==1and |
So I suggest to modify Lines 931 - 935 to
ifself._face_is_mappedandself._mapped_colorsisnotNone:self._facecolors=self._mapped_colorselse:self._set_facecolor(self._original_facecolor)ifself._edge_is_mappedandself._mapped_colorsisnotNone:self._edgecolors=self._mapped_colors
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.
Actually, line 913 in the green could be replaced with
if self._face_is_mapped or self._edge_is_mapped:because the check forself._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 thecheck_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 theself._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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 in315ff06
but as the comment wonders, doesnot 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 = True and 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._linewidths but 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 inmatplotlib#18480, and not copying them can mean that an artistthat used `update_from` could get in an inconsistent state.For example, this will fix legends of colour-mapped scatter plots, wherea copy of the scatter Collection is made to go in the legend.Fixesmatplotlib#19779.
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 ofed6d92b, 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).