Improve Line2D and MarkerStyle instantiation #6694

Kojoley · Jul 5, 2016

This reduces MarkerStyle._recache() calls from 4 to 1 in Line2D instantiation

I do not like this much. Otherwise we can extract body of set_fillstyle without _recache() call to an internal method and call it from __init__ and set_fillstyle.

Maybe add a _defer_recache=False kwarg to both set_marker and set_fillstyle? That way in __init__ you can prevent it from running.

A more invasive (but maybe better?) solution could be to use the stale/invalid pattern and then in all of the get_* methods check if it is stale and if so recache it.

It is probably better from a readability stand point to initialize all of the state here, rather than relying on a call to _recache to set it all up.

Kojoley · Jul 6, 2016

I have pushed alternative solution for double _recache() call elimination. It looks better than previous one and what your have suggested.

What about moving out initialization from _reacache to __init__, it is useless unless the _set_* functions depend on such behavior.

tacaswell · Jul 6, 2016

I suspect that not all of the _marker_function functions set all of the state they need to and _rechache is going back to the defaults.

Kojoley · Jul 6, 2016

Yes, I can confirm this as I have already tried it.

mdboom added the needs_review label Jul 5, 2016

tacaswell added this to the 2.1 (next point release) milestone Jul 6, 2016

tacaswell removed the needs_review label Jul 12, 2016

Kojoley deleted the Kojoley:improve-line2d-and-markerstyle-instantiation branch Jul 12, 2016

		Kojoley	Do not call `_recache()` twice in `MarkerStyle` constructor			`2f77749`
		Kojoley	Pass `marker` and `fillstyle` to `MarkerStyle` constructor		Some checks were not successful 1 errored, 1 failing, and 1 successful checks continuous-integration/appveyor/pr — AppVeyor build cancelled Details coverage/coveralls — Coverage decreased (-0.008%) to 70.268% Details continuous-integration/travis-ci/pr — The Travis CI build passed Details	`d24f2d5`

@@ -229,7 +230,9 @@ def set_fillstyle(self, fillstyle):
                  raise ValueError("Unrecognized fillstyle %s"
                                   % ' '.join(self.fillstyles))
              self._fillstyle = fillstyle
-    -        self._recache()
+    +        # do not call _recache() if marker is not yet set up
+    +        if self._marker is not None:

@@ -175,11 +175,12 @@ def __init__(self, marker=None, fillstyle=None):
              fillstyle : string, optional, default: 'full'
                  'full', 'left", 'right', 'bottom', 'top', 'none'
              """
-    -        # The fillstyle has to be set here as it might be accessed by calls to
-    -        # _recache() in set_marker.
-    -        self._fillstyle = fillstyle
-    -        self.set_marker(marker)
+    +        # The _recache() is called in both set_fillstyle() and set_marker()
+    +        # methods that's why here we have to have either marker or fillstyle
+    +        # preinitialized before setting other.
+    +        self._marker = None

matplotlib/matplotlib

Improve Line2D and MarkerStyle instantiation #6694

Kojoley commented Jul 5, 2016

Kojoley added some commits Jul 5, 2016

mdboom added the needs_review label Jul 5, 2016

Kojoley and 1 other commented on an outdated diff Jul 5, 2016

tacaswell commented on an outdated diff Jul 6, 2016

Kojoley commented Jul 6, 2016 • edited

tacaswell commented Jul 6, 2016

tacaswell added this to the 2.1 (next point release) milestone Jul 6, 2016

Kojoley commented Jul 6, 2016

tacaswell merged commit `7f4e84c` into matplotlib:master Jul 12, 2016
2 of 3 checks passed

2 of 3 checks passed

tacaswell removed the needs_review label Jul 12, 2016

Kojoley deleted the Kojoley:improve-line2d-and-markerstyle-instantiation branch Jul 12, 2016

matplotlib/matplotlib

Join GitHub today

Improve Line2D and MarkerStyle instantiation #6694

Conversation

Kojoley commented Jul 5, 2016

Kojoley added some commits Jul 5, 2016

Some checks were not successful

mdboom added the needs_review label Jul 5, 2016

Show 2 comments Hide comments Kojoley and 1 other commented on an outdated diff Jul 5, 2016

Kojoley Jul 5, 2016 • edited

tacaswell Jul 6, 2016

Some checks were not successful

Show 1 comment Hide comment tacaswell commented on an outdated diff Jul 6, 2016

tacaswell Jul 6, 2016

Some checks were not successful

Kojoley commented Jul 6, 2016 • edited

tacaswell commented Jul 6, 2016

tacaswell added this to the 2.1 (next point release) milestone Jul 6, 2016

Kojoley commented Jul 6, 2016

Hide details View details tacaswell merged commit 7f4e84c into matplotlib:master Jul 12, 2016 2 of 3 checks passed

2 of 3 checks passed

tacaswell removed the needs_review label Jul 12, 2016

Kojoley deleted the Kojoley:improve-line2d-and-markerstyle-instantiation branch Jul 12, 2016

Kojoley and 1 other commented on an outdated diff Jul 5, 2016

tacaswell commented on an outdated diff Jul 6, 2016

tacaswell merged commit `7f4e84c` into matplotlib:master Jul 12, 2016
2 of 3 checks passed