Fix redundent set line props #6175
Conversation
|
@madphysicist Sorry to have stolen this from you if you were excited about doing it 😞 |
|
I was actually really hoping to avoid this one :) |
|
LGTM FWIW |
|
There is also some possibly subtle issues that may pop up because this is switch from using @madphysicist It is worth something. |
| @@ -275,10 +271,10 @@ def _setdefaults(self, defaults, *kwargs): | |||
| def _makeline(self, x, y, kw, kwargs): | |||
| kw = kw.copy() # Don't modify the original kw. | |||
| kwargs = kwargs.copy() | |||
| default_dict = self._getdefaults(None, kw, kwargs) | |||
| self._setdefaults(default_dict, kw, kwargs) | |||
| kw.update(kwargs) | |||
There was a problem hiding this comment.
I have pretty extensive notes about keeping kw and kwargs distinct. I can almost guarantee you that this breaks stuff. All my notes about this can be found in the long comments nearby this area of code.
There was a problem hiding this comment.
Most of those comments are on the _makefill related code which looks much worse.
In this case the user kwargs were just getting passed to line.set which just farms them back out to line.set_*(...). Now the same thing is happening (through update which does the same dispatching, but does some default cleaning first (which strips the Nones)).
|
The 2.6 test failure looks like it is something I put in with a backport, looking into that now too. |
|
@WeatherGod Can you give this a second look? |
| @@ -275,10 +271,10 @@ def _setdefaults(self, defaults, *kwargs): | |||
| def _makeline(self, x, y, kw, kwargs): | |||
| kw = kw.copy() # Don't modify the original kw. | |||
| kwargs = kwargs.copy() | |||
There was a problem hiding this comment.
There is no point in copying kwargs here.
These changes are to the methods on _process_plot_var_args In `_makelines` we get two dictionaries in from `_plot_args` which map to the keywords we generate internally and the kwargs passed in by the user. Previously the internally generated kwargs were passed to the Line2D `__init__` and the user supplied kwargs were passed to (eventually) `line.set` to bulk update the properties of the line. The change to `Line2D` is because `set_linestyle` internally calls `set_drawstyle`. What would happen in the case of `step` where the linestyle is `step-pre-` with the changes to `_base`: - set_linestyle would set the draw style to 'step' - set_drawstyle would re-set it to the default value Previously, both of these would be called in `__init__` with the default values and the updated with the user supplied values which masked the order issue in `__init__` (as most `Line2D` artists are made through the plot interface). closes matplotlib#6173
Because the drawstyle can be set via both the drawstyle and via linestyle do some checking / normalize in the init. Pulled the code that is shared between the `__init__` and `set_linestyle` into a new private method. Reverts minor change to order of operations
tacaswell
force-pushed
the
fix_redundent_set_line_props
branch
from
790c4ac to
3cada62
Compare
'quick' fix to #6173
Found one live 🐲 and one sleeping 🐉
This is targeted at 1.5.x, but I would be very easily convinced that these changes are too big to apply to 1.5.x and needs to be targeted at v2.x or master.
There is also an issue that
lsandlinestyleare not properly aliased, but that should be fixed on master using https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/cbook.py#L2449