Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepare for ragged array warnings in NumPy 1.19 #17289

Merged
merged 7 commits into from May 22, 2020

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented May 1, 2020

PR Summary

NumPy 1.19 will issue a deprecation warning about ragged arrays numpy/numpy#15119, which causes our tests to fail as we fail on warning. This fixes the easy things, but there are 4 more classes of failures:

  • cbook._reshape_2D's test checking that a ragged array is turned into an object array. Fixes BUG: VisibleDeprecationWarning in boxplot #16353
  • hist casting ragged x input to a 2d array (most of the failures are this one)
    • this should be fixed by the first one
  • test_scatter_marker passes an array of color-likes, which goes through _combine_masks which tries to cast as an array
    • We could tell _combine_masks to use object array, or pass a flag to allow it? I'm not sure if it should always try to use an object array.
  • test_bad_plot_args passes some ridiculous values, and I don't know if they should be checked earlier or just allow it to fail when NumPy turns this into an error

I haven't just changed these to explicitly ask for object arrays because I haven't yet figured out what the implications are in NumPy and why they started warning.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • [N/A] New features are documented, with examples if plot related
  • [N/A] Documentation is sphinx and numpydoc compliant
  • [N/A] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • [N/A] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@tacaswell
Copy link
Member

Do you want to get these is as-is and address the rest later or hold these to fix them all in one shot?

@QuLogic
Copy link
Member Author

QuLogic commented May 5, 2020

Ah, telling _reshape_2D to use object dtype breaks violinplot instead...

@QuLogic QuLogic marked this pull request as ready for review May 6, 2020 04:58
@QuLogic
Copy link
Member Author

QuLogic commented May 6, 2020

This is mostly done, except for test_bad_plot_args, and I'm not sure what to do there. That test does:

plt.plot((np.arange(5).reshape((1, -1)), np.arange(5).reshape(-1, 1)))

which is of course, non-sensical. It currently checks that it raises a ValueError. Unlike the other inputs, which raise in _plot_args, this one actually works fine, and raises somewhere deep in Line2D basically trying to draw. I think this case should have been caught in _plot_args, but I'm not sure how to check it safely.

@QuLogic
Copy link
Member Author

QuLogic commented May 16, 2020

Not sure if that's the best last commit, but that should fix all the warnings.

@tacaswell
Copy link
Member

I pushed a commit to fix test_violinplot_pandas_series[png] that was failing for me locally.

@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label May 16, 2020
lib/matplotlib/cbook/__init__.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
@tacaswell
Copy link
Member

Do we want to backport this to 3.2.x?

x = np.asanyarray(x)
try:
x = np.asanyarray(x)
except np.VisibleDeprecationWarning:
Copy link
Member

Choose a reason for hiding this comment

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

So, what happens after the deprecation period?

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume it'll be a ValueError, but it wasn't clear in the NEP.

QuLogic and others added 7 commits May 21, 2020 23:08
The `segments` parameter is a list of lines (a line being a 2D array or
list of 2-tuples), but every line is not required to be the same length.
This would cause NumPy to produce an object array, but it will start to
warn about ragged arrays in 1.19.

An array isn't really needed as `Line3DCollection._segments3d` is only
iterated over.
These inputs may be ragged, but we only just need to know the length of
the items. There's no need to make an array to check that.
This is a raising a deprecation warning in NumPy 1.19, may go away some
time later, and is not strictly necessary for the implementation.
This causes a NumPy deprecation warning if the list is ragged.
This function accepts almost anything list-like, so we really do want
conversion to object arrays if the input is ragged. For example, this
might occur for `scatter()` if passed different types of `colors`, like
in the `test_scatter_marker` test.
@jklymak jklymak merged commit 5ab1352 into matplotlib:master May 22, 2020
@jklymak
Copy link
Member

jklymak commented May 22, 2020

Oh, hmmm, didn't notice that had a bunch of commits. Sorry if that was meant to be squashed...

@QuLogic QuLogic deleted the np119 branch May 22, 2020 05:07
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Jun 10, 2020
Prepare for ragged array warnings in NumPy 1.19
Conflicts:
	lib/matplotlib/axes/_axes.py
         - implicitly backported changes to wording in error messages
@tacaswell
Copy link
Member

Do we want to backport this to 3.2.2? I tried cherry-picking and there is 12 failures on 3.2.x with this backported and numpy master.

I guess our choices are :

  1. backport nothing
  2. backport, but don't clean up the other failures (most of them look like they are in scatter's input munging)
  3. backport and clean up all the other warnings.

I'm between 1 and 3. Doing it half way seems like a poor life choice and I am not sure that it is worth the effort for 3.

@QuLogic
Copy link
Member Author

QuLogic commented Jun 11, 2020

At least for Fedora, this would not be a problem as they would not show up at the same time. We can possibly just say they aren't supported on other systems (mainly conda-forge, I'd assume).

I tried the backport and there are about 11 extra failures. I believe that most of them are fixed by #15834, as that removes the np.array call, but I have not tested it out, as I'm not sure we want to actually backport that.

@tacaswell
Copy link
Member

Given that this caused a regression I am glad that we opted not to backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: VisibleDeprecationWarning in boxplot
4 participants