Skip to content

Improve Line2D and MarkerStyle instantiation #6694

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

Kojoley
Copy link
Member

@Kojoley Kojoley commented Jul 5, 2016

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

@@ -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:
Copy link
Member Author

@Kojoley Kojoley Jul 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Because `set_marker` calls it by itself
# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Kojoley force-pushed the improve-line2d-and-markerstyle-instantiation branch from 1ba7be6 to 486ce4d Compare July 6, 2016 16:29
@Kojoley
Copy link
Member Author

Kojoley commented 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
Copy link
Member

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.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Jul 6, 2016
@Kojoley
Copy link
Member Author

Kojoley commented Jul 6, 2016

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

@tacaswell tacaswell merged commit 7f4e84c into matplotlib:master Jul 12, 2016
@Kojoley Kojoley deleted the improve-line2d-and-markerstyle-instantiation branch July 12, 2016 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants