@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 checksin 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 coloror 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 theplot 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 "ifargs[1] is a valid format string" determine the arguments (instead of depending on "ifargs[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 inplot()) 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 andarray and it hasdtype.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 undertest_axes.py? Currently,_plot_args_replacer seems to be lightly tested intest_preprocess_data.py.

Thank you!

@zhangeugenia I think I would putdirect 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

ifnot (args[1]indataorhasattr(data,'dtype')andargs[1]indata.dtype.names):

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

Additional note: ndarrays, structured or not, do have acontains 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 usingin on stuctured arrays if any new features in the future required usingin ondata. It seems like usingin ondata 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 fordata.dtype.names was added into the conditional as well, since a non-structured numpy array would givedata.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!