Clarify the implementation of _process_plot_var_args. #12969

anntzer · 2018-12-10T08:20:59Z

Having an argument named kwargs that's not a dict but a tuple is just
a good way to trip the reader. Fortunately _getdefaults and
_setdefaults in only even called with a single kwargs, so we can replace
it with a single non-varargs argument and simplify the code at the same
time.

PR Summary

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

timhoffm · 2018-12-10T19:10:00Z

lib/matplotlib/axes/_base.py

-        returned dictionary.
-
+        If some keys in the property cycle (excluding those in *ignore*) are
+        absent or set to None in *kw*, return a copy of the next entry in the


Suggested change

absent or set to None in *kw*, return a copy of the next entry in the

absent or set to None in the dict *kw*, return a copy of the next entry in the

timhoffm · 2018-12-10T19:11:29Z

lib/matplotlib/axes/_base.py

-        update those other dictionaries with information in defaults if
-        none of the other dictionaries contains that information.
-
+        Add to *kw* the entries in *default* that are absent or set to None in


Suggested change

Add to *kw* the entries in *default* that are absent or set to None in

Add to the dict *kw* the entries in the dict *default* that are absent or set to None in

timhoffm · 2018-12-10T19:12:56Z

lib/matplotlib/axes/_base.py

-        an empty dictionary. Ignored keys are excluded from the
-        returned dictionary.
-
+        If some keys in the property cycle (excluding those in *ignore*) are


Suggested change

If some keys in the property cycle (excluding those in *ignore*) are

If some keys in the property cycle (excluding those in the set *ignore*) are

timhoffm · 2018-12-10T19:13:46Z

lib/matplotlib/axes/_base.py


    def _makeline(self, x, y, kw, kwargs):
        kw = {**kw, **kwargs}  # Don't modify the original kw.
-        default_dict = self._getdefaults(None, kw)
+        default_dict = self._getdefaults(set(), kw)


Suggested change

default_dict = self._getdefaults(set(), kw)

default_dict = self._getdefaults(ignore=set(), kw=kw)

The function is defined just above, and it's not as if you can really guess what it does from the name, so in practice you're going to scroll up and see what it does anyways.

timhoffm · 2018-12-10T19:22:40Z

lib/matplotlib/axes/_base.py

-            if all(kw.get(k, None) is None for kw in kwargs):
-                for kw in kwargs:
-                    kw[k] = defaults[k]
+            if kw.get(k, None) is None:


thats

kw.update({k: v for k, v in defaults.items() if kw.get(k) is None})

Not sure if that is more readable

You can bet that if anything is writable as a comprehension I will likely have tried doing so. In this case I thought the appearance of kw twice, outside and inside, hinders legibility.

The double appearance in the expression is inherent to the problem. Essentially what we have todto (and what the expression also says):

Update kw
with keys and values from defaults
where the key is not in kw or maps to None

The more I think about it, the more I like the comprehension 😄.

Note also that kw.get(k, None) is the same as kw.get(k).

Actually I thought the clearest was way to just drop out the Nones first, ie

def _getdefaults(self, ignore, kw): """ If some keys in the property cycle (excluding those in the set *ignore*) are absent or set to None in the dict *kw*, return a copy of the next entry in the property cycle, excluding keys in *ignore*. Otherwise, don't advance the property cycle, and return an empty dict. """ prop_keys = self._prop_keys - ignore keys_no_none = {k for k, v in kw.items() if v is not None} if prop_keys - keys_no_none: # Some keys in prop_keys are not in kw. # Copy the default dict to avoid modifying the reference held by # the cycler. default_dict = next(self.prop_cycler).copy() for p in ignore: default_dict.pop(p, None) else: default_dict = {} return default_dict

and

def _setdefaults(self, defaults, kw): """ Add to the dict *kw* the entries in the dict *default* that are absent or set to None in *kw*. """ kw_no_none = {k: v for k, v in kw.items() if v is not None} kw.clear() kw.update(defaults) kw.update(kw_no_none)

How does that look to you?

Actually, self._setdefaults(default_dict, kw) is basically kw = {**default_dict, **kw}. So if we can replace the kw instance by a new one (which I suppose we can), we can get rid of the function alltogether. Or you could use a collections.ChainMap.

Better but still a bit verbose. It's less efficient than the comprehension, but with few keywords that probably doesn't matter.

What about replacing the function with:

def _no_none(kw): return {k: v for k, v in kw.items() if v is not None} kw = {**defaults, **_no_none(kw)}

or ChainMap(defaults, _no_none(kw))?

Indeed, introducing the _no_none helper makes the whole thing much simpler. Pushed a new version.

Actually, that doesn't work: we don't want to drop all the None entries in kw, only those that are present in defaults. Those that are not in defaults should be kept.
(This is needed e.g. for the closed kwarg to fill.)

Returning to the previous version...

Having an argument named `kwargs` that's not a dict but a tuple is just a good way to trip the reader. Fortunately _getdefaults and _setdefaults in only even called with a single kwargs, so we can replace it with a single non-varargs argument and simplify the code at the same time.

timhoffm · 2018-12-11T00:08:40Z

lib/matplotlib/axes/_base.py

@@ -391,8 +371,6 @@ def _grab_next_args(self, *args, **kwargs):


 class _AxesBase(martist.Artist):
-    """


Was this maybe here to prevent docstring inheritance? OTOH, doesn't make much sense since the class is private and thus neither shows up in the HTML docs nor should a user encounter it in an interactive session. So probably fine.

timhoffm

Actually, that doesn't work: we don't want to drop all the None entries in kw, only those that are present in defaults. Those that are not in defaults should be kept.
(This is needed e.g. for the closed kwarg to fill.)

Returning to the previous version...

Good catch. Unfortunately, you're right.

We have sought for better solutions enough. Thus approving as is (and pushing my first-reviewer count 😄 ).

anntzer added this to the v3.1 milestone Dec 10, 2018

timhoffm reviewed Dec 10, 2018

View reviewed changes

anntzer force-pushed the getsetdefaults branch 2 times, most recently from 3a2abb5 to d0d36cb Compare December 10, 2018 23:32

timhoffm reviewed Dec 11, 2018

View reviewed changes

anntzer force-pushed the getsetdefaults branch from d0d36cb to 3a2abb5 Compare December 11, 2018 00:20

timhoffm approved these changes Dec 11, 2018

View reviewed changes

jklymak approved these changes Jan 2, 2019

View reviewed changes

jklymak merged commit e8be5f7 into matplotlib:master Jan 2, 2019

anntzer deleted the getsetdefaults branch January 2, 2019 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify the implementation of _process_plot_var_args. #12969

Clarify the implementation of _process_plot_var_args. #12969

anntzer commented Dec 10, 2018

timhoffm Dec 10, 2018

anntzer Dec 10, 2018

timhoffm Dec 10, 2018

anntzer Dec 10, 2018

timhoffm Dec 10, 2018

anntzer Dec 10, 2018

timhoffm Dec 10, 2018

anntzer Dec 10, 2018

timhoffm Dec 10, 2018

timhoffm Dec 10, 2018 •

edited

Loading

anntzer Dec 10, 2018

timhoffm Dec 10, 2018 •

edited

Loading

anntzer Dec 10, 2018

timhoffm Dec 10, 2018 •

edited

Loading

timhoffm Dec 10, 2018 •

edited

Loading

anntzer Dec 10, 2018

anntzer Dec 11, 2018

timhoffm Dec 11, 2018

timhoffm left a comment •

edited

Loading

	absent or set to None in kw, return a copy of the next entry in the
	absent or set to None in the dict kw, return a copy of the next entry in the

	Add to kw the entries in default that are absent or set to None in
	Add to the dict kw the entries in the dict default that are absent or set to None in

	If some keys in the property cycle (excluding those in ignore) are
	If some keys in the property cycle (excluding those in the set ignore) are

	default_dict = self._getdefaults(set(), kw)
	default_dict = self._getdefaults(ignore=set(), kw=kw)

		@@ -391,8 +371,6 @@ def _grab_next_args(self, args, *kwargs):


		class _AxesBase(martist.Artist):
		"""

Clarify the implementation of _process_plot_var_args. #12969

Clarify the implementation of _process_plot_var_args. #12969

Conversation

anntzer commented Dec 10, 2018

PR Summary

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

timhoffm Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm left a comment • edited Loading

Choose a reason for hiding this comment

timhoffm Dec 10, 2018 •

edited

Loading

timhoffm Dec 10, 2018 •

edited

Loading

timhoffm Dec 10, 2018 •

edited

Loading

timhoffm Dec 10, 2018 •

edited

Loading

timhoffm left a comment •

edited

Loading