Cleanup _plot_args_replacer logic #10872
Conversation
8fc43eb to
80cddbb
Compare
I'm worried this might mess categorical axes. I'll make a note to check on that. |
|
This will 'work' if you do |
|
Why do we not want to pull out the data as part of the test? (i.e. try |
|
"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 to So I'm +1 on try |
|
Is there more support for "try |
|
h5py objects can be slow. I would lean towards continuing to use |
|
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. |
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``). | |||
There was a problem hiding this comment.
This is inconsistent with the actual test, and with the desired behavior; the structured ndarray does not support the membership test.
There was a problem hiding this comment.
Maybe should say 'support <arg> in data or be a numpy structured array'?
|
I don't mind the rebase, but note that #10928 will probably make all this obsolete. |
|
Don't we need both? |
|
Note: Adapted |
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:
name 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 if
data[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.