NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

Cleanup _plot_args_replacer logic#10872

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

phobson merged 2 commits intomatplotlib:masterfromtimhoffm:arg-replacer

Jul 10, 2018

Merged

Cleanup _plot_args_replacer logic#10872

phobson merged 2 commits intomatplotlib:masterfromtimhoffm:arg-replacer

Jul 10, 2018

Conversation

Copy link

Member

timhoffm commentedMar 23, 2018•
edited
Loading

PR Summary

This is a follow-up to#10810. Basically it cleans up the code structure. The logic in the PR is the same, just rewritten to be more clear.

Additional follow up question

As part of the cleanup, I realized that we are not exactly checking what we want to know:

Want to know whetherdata as an item namedname.
Checkname in data or name in data.dtype.names.

That works for usual data (dict like and numpy.array), but may fail e.g. if a user would use a string-list for data. I assume that's ok-ish.

I think there is no real "has_item()" function and the only exact way of finding out ifdata[name] works is really trying and catching a possible exception. I assume we don't want to really pull data as part of this check.

So in total the present check is still the check we want to do. Please correct me if I'm wrong here.

timhoffm force-pushed thearg-replacer branch 2 times, most recently from8fc43eb to80cddbbCompare

March 23, 2018 07:48

timhoffm added this to thev3.0 milestone

Mar 26, 2018

Copy link

Member

phobson commentedMar 26, 2018

may fail e.g. if a user would use a string-list for data. I assume that's ok-ish.

I'm worried this might mess categorical axes. I'll make a note to check on that.

Copy link

Member

tacaswell commentedMar 28, 2018

This will 'work' if you doax.plot('foo', 'bar', data=['foo', 'bar'])

tacaswell approved these changes

Mar 28, 2018

View reviewed changes

Copy link

Contributor

anntzer commentedMar 28, 2018

Why do we not want to pull out the data as part of the test? (i.e. trydata[name] and catch attributeerror) "Should" be cheap...

Copy link

MemberAuthor

timhoffm commentedMar 28, 2018•
edited
Loading

"Probably" you are right.

In the worst case, it could get a factor 2 slower because we'll have to fetch the data later anyway. But I think this is negligible compared to all the other stuff done during plotting. If someone has data with a slow getitem, they can still resort toplot(data['x'], data['y']) to only access them once.

So I'm +1 on trydata[name] and catch AttributeError.

Copy link

MemberAuthor

timhoffm commentedMar 28, 2018•
edited
Loading

Is there more support for "trydata[name] and catch AttributeError"? If so, I would create an alternative PR.

Copy link

Member

tacaswell commentedMar 28, 2018

h5py objects can be slow.

I would lean towards continuing to use__contains__ and documenting that support that is part of the required API on the things passed in todata=.

cleanup _plot_args_replacer logic

31dbef6

timhoffm force-pushed thearg-replacer branch from80cddbb to31dbef6Compare

March 31, 2018 12:01

Copy link

MemberAuthor

timhoffm commentedMar 31, 2018

If h5py is the couter-example that item access can be slow, I'd leave the implementation logic as is, just with the cleaned up code from this PR.

I've added a note that the data object must support membership test.

tacaswell approved these changes

Mar 31, 2018

View reviewed changes

efiring reviewed

Mar 31, 2018

View reviewed changes

lib/matplotlib/__init__.py Outdated

		@@ -1529,6 +1529,9 @@ def _replacer(data, key):
		following arguments are replaced by data[<arg>]:

		{replaced}

		Objects passed as data must support item access (``data[<arg>]``) and
		membership test (``<arg> in data``).

Copy link

Member

efiringMar 31, 2018

Choose a reason for hiding this comment

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

This is inconsistent with the actual test, and with the desired behavior; the structured ndarray does not support the membership test.

Copy link

Member

tacaswellMar 31, 2018•
edited
Loading

Choose a reason for hiding this comment

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

Maybe should say 'support<arg> in data or be a numpy structured array'?

Copy link

Contributor

anntzer commentedMar 31, 2018

I don't mind the rebase, but note that#10928 will probably make all this obsolete.

Copy link

Member

tacaswell commentedMar 31, 2018

Don't we need both?

Copy link

Contributor

anntzer commentedMar 31, 2018•
edited
Loading

#10928 kills _plot_args_replacer and uses the standard _replacer logic used by everyone else (try to get the item and if it's there, swap it in -- so access is only done one anyways)

(and yes this PR is the original reason why I did#10928)