Fix for plt.plot() does not support structured arrays as data= kwarg - #10810

@tacaswell, do you want to stick one of the version 2 milestones on this? It is addressing a bug milestoned for 2.2, and it looks like a clean and simple fix.

Inclined to not backport, this never worked (so not fixing a regression), is not a failure to import / segfault, and the user has a clear work around (just don't use the data kwarg with structured arrays).

The issue here is that structured arrays support __getitem__ but not __contains__ so this only checks in if the key is in the data if it also parses as a color? There will still be a slight behavior difference between structure arrays and other containers in that structured arrays will not get the warning in the case of a conflict and (I think) the color behaviour will win!

Does this change how things fail if you pass in keys that are not a color or in the container?

@tacaswell, thank you, I had not looked into the situation in enough detail. It sounds like the real solution for fully supporting structured arrays would be to make _preprocess_data substitute a minimal wrapper for a structured 'data' array that would supply the missing __contains__, or, to handle at least the plot case, to put in special-case handling of the 'in' test if 'data' is a structured array.

Thanks for working on this @zhangeugenia . Despite my seemingly negative comments, I think this is definitely going in the right direction and appreciate your work!

Hello! Thank you all for your time and feedback, I greatly appretiate it (especially since plot() is a very important function)!

As @tacaswell mentioned, this PR does indeed change the behaviour of _plot_args_replacer. In the PR, the function now depends on "if args[1] is a valid format string" determine the arguments (instead of depending on "if args[1] is in the container or not") when there are 2 arguments passed in. I am assuming that it would be best to keep the original behaviour to keep things consistent, so the ordering/behaviour will be reverted back to how it was originally in the changes.

Out of the two methods that @efiring proposed to support structured arrays, the latter (to check the type of data, and invoke __getitem__ in place of __contains__ if data is a structured array in plot()) would be the quick and easy fix. Personally, I'm more inclined to the former (adding a wrapper class to a structured array to implement __contains__) since it could be reused in future development. After a bit of looking around the Numpy documentation I think that we can identify a structured array if it is a ndarray and it has dtype.fields, but I think I may need to look into the source code for the object in more detail.

I do have a question regarding testing though - once the support for structured arrays has been implemented, if I were to add tests to test _plot_args_replacer directly would it still go under test_axes.py? Currently, _plot_args_replacer seems to be lightly tested in test_preprocess_data.py.

Thank you!

@zhangeugenia I think I would put direct tests of _plot_args_replacer either in test_axes.py or, if they are extensive, in a new "test_plot_args_replacer.py".

Going the subclass route might not be a good idea--I suspect it would be overkill, and it is probably much trickier than one might expect. Is there likely to be any other place where it would help enough to justify the fuss?

All you need for option 1 is something like

if not (args[1] in data
        or hasattr(data, 'dtype') and args[1] in data.dtype.names):

To be even safer, in case something comes through with a 'dtype' that is not a standard numpy dtype, you could insert and hasattr(data.dtype, 'names') in the middle of the second line above.

Additional note: ndarrays, structured or not, do have a contains method, but it doesn't do what we need here.

I like @efiring 's suggestion.

I am hesitant to add direct tests of _plot_args_replacer, it is better to test it through usage of the public API. I suspect that in the near future the decorator will be made python3 only and made public.

Thank you for your guidance! Indeed, a wrapper class would be a lot of work, I just thought it would reduce issues with using in on stuctured arrays if any new features in the future required using in on data. It seems like using in on data is fairly uncommon, so the lack of use certainally would not justify the wrapper class in this case.

I will take @efiring 's suggestion and make the needed changes to _plot_args_replacer, and test it via a public method as @tacaswell suggested. Thank you so much once again!

Updated to use @efiring 's suggestion! A check for data.dtype.names was added into the conditional as well, since a non-structured numpy array would give data.dtype.names = None. Please let me know if there are any further changes needed/a squish needed, or if I should remove the test from test_axis. Thank you!

Looks good to me!

Thank you, @zhangeugenia!

Thanks @zhangeugenia and congratulations on what is (I think?) your first merged Matplotlib PR 🎉 Hopefully we will hear from you again!