MNT: unify code path of set, update, setp by tacaswell · Pull Request #5599 · matplotlib/matplotlib

tacaswell · 2015-12-01T16:32:57Z

Single code path for the property -> setter lookup logic.

tacaswell · 2015-12-01T16:35:07Z

A follow on to this might be to implement a priority lookup to sort the key/value pairs instead of alphabetical sorting, but I am not sure if that is replicating functionality already in traitlets.

WeatherGod · 2015-12-01T16:41:55Z

I would be +1 on implementing a proper lookup order. The reversed(IIRC)
alphabetical order was a quick-n-dirty solution that I used because it just
so happened that 'edgecolor' > 'color' and such. We won't always be quite
so lucky...

On Tue, Dec 1, 2015 at 11:35 AM, Thomas A Caswell [email protected]
wrote:

A follow on to this might be to implement a priority lookup to sort the
key/value pairs instead of alphabetical sorting, but I am not sure if that
is replicating functionality already in traitlets.

—
Reply to this email directly or view it on GitHub
#5599 (comment)
.

mdboom · 2015-12-01T16:47:31Z

~~loop up how to~~ ? (I think?) The sentence is a little unclear.

fixed and force-pushed

Single code path for the property -> setter lookup logic.

mdboom · 2015-12-01T16:48:29Z

Maybe this could be an embedded function in update(), since it isn't used from anywhere else?

good point, when I started I didn't think I would be able to use update everywhere else.

on second thought, wouldn't that result in re-defining the function everytime through call to update?

No -- Python will compile the function only once -- it only redefines the local variables each time.

mdboom · 2015-12-01T16:51:38Z

The sorting by key name is just to prevent any non-deterministic behavior?

and makes color/facecolor/edgecolor go in the right order.

Partly. It is also to have a priority structure. If someone defines both
"color" and "facecolor" at the same time, the current behavior is for color
to take precedence over facecolor, IIRC.

On Tue, Dec 1, 2015 at 11:51 AM, Michael Droettboom <
[email protected]> wrote:

In lib/matplotlib/artist.py
#5599 (comment):

@@ -845,21 +845,46 @@ def update(self, props):
"""
store = self.eventson
self.eventson = False

changed = False

try:

ret = [self._update_property(k, v)

for k, v in sorted(props.items(), reverse=True)]

The sorting by key name is just to prevent any non-deterministic behavior?

—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/5599/files#r46304017.

mdboom · 2015-12-01T16:55:19Z

This is cool, and I'm always happy to see PRs that reduce total lines of code ;)

Maybe this is a minor point, but it seems that setp is really the most general of the three, since it can do both an ordered list (in *args) and an unordered dictionary (in **kwargs), so maybe that should be the one that the other two call? That would prevent the need for passing one entry at a time from setp to update.

Hide private helper method more thoroughly.

tacaswell · 2015-12-01T17:02:27Z

I changed a list to a set literal, this can not be back-ported to 1.5.x.

WeatherGod · 2015-12-01T17:02:45Z

Agree with @mdboom. Let setp be the only place where sorting happens. I
haven't looked carefully enough, but I wonder if there are subtle
differences in behavior now.

On Tue, Dec 1, 2015 at 11:55 AM, Michael Droettboom <
[email protected]> wrote:

This is cool, and I'm always happy to see PRs that reduce total lines of
code ;)

Maybe this is a minor point, but it seems that setp is really the most
general of the three, since it can do both an ordered list (in _args) and
an unordered dictionary (in *_kwargs), so maybe that should be the one
that the other two call? That would prevent the need for passing one entry
at a time from setp to update.

—
Reply to this email directly or view it on GitHub
#5599 (comment)
.

restore old behavior

Make one call to `update` with the ordered dict rather than one call per element.

Add the ability to set the precedence of properties when updating multiple properties on once. This adds a class-level attribute `Artist._prop_order` which is a dictionary keyed on property names with integer values. When using `set` or `setp` the properties will be applied in descending order by value then by name.

mdboom · 2015-12-01T19:49:06Z

If we're concerned about subtle changes in behavior, the new zero-tolerance test suite seems to be pretty good at rooting that out. @tacaswell: if you wanted, you could base this on my zero tolerance branch and see if any additional tests are failing beyond the dozen or so that are failing on that branch presently.

tacaswell · 2015-12-01T19:53:49Z

setp is doing a bunch of extra work. IIRC the ArtistInpsector code is slow (I think the call to kwdoc is one of the more expensive parts of our import time) and we call update many places.

Ended up landing on update -> no sorting, but does the right thing if you pass it an OrderedDict, set -> sorts based on the new _prop_order dict then on key. setp uses both.

mdboom · 2015-12-01T20:00:06Z

returna -> return

tacaswell · 2015-12-01T20:12:27Z

I rebased it locally and have the same 11 failures.

tacaswell · 2015-12-03T03:19:20Z

Any further thoughts on this? I am tempted to rip the prop_order change back out and give that a more through airing on it's own. I am not 100% sure we will need it with traitlets and I don't want to put it in for 2.0 just to take it out in 2.1.

MNT: unify code path of set, update, setp

tacaswell added this to the next major release (2.0) milestone Dec 1, 2015

tacaswell added the status: needs review label Dec 1, 2015

mdboom reviewed Dec 1, 2015
View reviewed changes

MNT: unify code path of set, update, setp

63f4437

Single code path for the property -> setter lookup logic.

mdboom reviewed Dec 1, 2015
View reviewed changes

tacaswell force-pushed the mnt_unify_artist_update-set branch from 4f8343b to 63f4437 Compare December 1, 2015 16:48

mdboom reviewed Dec 1, 2015
View reviewed changes

tacaswell added 2 commits December 1, 2015 11:58

MNT: move helper function into update

3401a5d

Hide private helper method more thoroughly.

DOC: more minor API constraints

c458cbc

tacaswell added 3 commits December 1, 2015 14:39

REV: do not sort by key in Artist.update

bba3ae2

restore old behavior

MNT: use ordereddict to order args in setp

0ea5fff

Make one call to `update` with the ordered dict rather than one call per element.

mdboom reviewed Dec 1, 2015
View reviewed changes

Comment thread doc/api/api_changes/2015-12-01-TAC.rst Outdated

mdboom Dec 1, 2015

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

returna -> return

DOC: fix typos

1905607

tacaswell force-pushed the mnt_unify_artist_update-set branch from 55370f8 to 1905607 Compare December 1, 2015 20:13

mdboom added a commit that referenced this pull request Dec 14, 2015

Merge pull request #5599 from tacaswell/mnt_unify_artist_update-set

16f4895

MNT: unify code path of set, update, setp

mdboom merged commit 16f4895 into matplotlib:v2.0.x Dec 14, 2015

mdboom removed the status: needs review label Dec 14, 2015

tacaswell deleted the mnt_unify_artist_update-set branch December 19, 2015 02:53

tacaswell mentioned this pull request Jul 7, 2016

Test failure with matplotlib 1.5.2rc2 on Debian pandas-dev/pandas#13570

Closed

Conversation

tacaswell commented Dec 1, 2015

Uh oh!

tacaswell commented Dec 1, 2015

Uh oh!

WeatherGod commented Dec 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdboom commented Dec 1, 2015

Uh oh!

tacaswell commented Dec 1, 2015

Uh oh!

WeatherGod commented Dec 1, 2015

Uh oh!

mdboom commented Dec 1, 2015

Uh oh!

tacaswell commented Dec 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Dec 1, 2015

Uh oh!

tacaswell commented Dec 3, 2015

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants