Fix legend of colour-mapped scatter plots.

[QuLogic]

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

• Has pytest style unit tests (and pytest passes).
• Is Flake 8 compliant (run flake8 on changed files to check).
• [n/a] New features are documented, with examples if plot related.
• [n/a] Documentation is sphinx and numpydoc compliant (the docs should build without error).
• Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
• [n/a] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
• [n/a] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

[tacaswell]

The issue is that this was coming back false, but we should have gone through this branch?

[smartlixx]

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

	if self._face_is_mapped:
	self._facecolors = self._mapped_colors
	else:
	self._set_facecolor(self._original_facecolor)
	if self._edge_is_mapped:
	self._edgecolors = self._mapped_colors
	else:
	self._set_edgecolor(self._original_edgecolor)

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

	if (len(paths) == 1 and len(trans) <= 1 and
	len(facecolors) == 1 and len(edgecolors) == 1 and

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_colors

[efiring]

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..

[efiring]

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.

[QuLogic]

So, it was bugging me a bit why this was triggered, so I spent a bit of time looking closer. The problem stems from Collection.update_from. In the issue, the legend causes a second PathCollection to be created, and the handler calls legend_handle.update_from(orig_handle). This does a bunch of copying:

matplotlib/lib/matplotlib/collections.py

Lines 942 to 962 in 315ff06

	def update_from(self, other):
	"""Copy properties from other to self."""
	artist.Artist.update_from(self, other)
	self._antialiaseds = other._antialiaseds
	self._original_edgecolor = other._original_edgecolor
	self._edgecolors = other._edgecolors
	self._original_facecolor = other._original_facecolor
	self._facecolors = other._facecolors
	self._linewidths = other._linewidths
	self._linestyles = other._linestyles
	self._us_linestyles = other._us_linestyles
	self._pickradius = other._pickradius
	self._hatch = other._hatch
	# 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.stale = True

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 update_from to copy the other internals, but I have not implemented that yet.

[QuLogic]

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?

[QuLogic]

OK, this now does as I said, changing Collection.update_from to copy the additional attributes that were added, so that we don't end up in some indeterminate state.

[tacaswell]

Presumably we want the same fix on the default branch and will handle that via the merge up?

[QuLogic]

It's fixed there because the _check_update was removed like this PR originally did, but yea, we'll want that there too.

[tacaswell]

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.

[efiring]

Thank you, @QuLogic, it was good that you took a closer look.