Skip to content

Fix reshape list of strings#18243

Merged
QuLogic merged 3 commits intomatplotlib:masterfrom
jklymak:fix-reshape-list-of-strings
Aug 13, 2020
Merged

Fix reshape list of strings#18243
QuLogic merged 3 commits intomatplotlib:masterfrom
jklymak:fix-reshape-list-of-strings

Conversation

@jklymak
Copy link
Copy Markdown
Member

@jklymak jklymak commented Aug 13, 2020

PR Summary

Closes #18234

cbook._reshape_2D was altered to check for iterables in a list, which is great, except categorical strings are iterables. This broke:

import matplotlib.pyplot as plt

fig, ax = plt.subplots()
n, bins, patches = ax.hist(['a', 'b', 'b', 'c', 'd', 'ff', 'ff', 'a'])
plt.show()

Before

Figure_2

After

Figure_1

PR Checklist

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

@jklymak jklymak force-pushed the fix-reshape-list-of-strings branch from e9efb6c to cd42a3f Compare August 13, 2020 16:45
@jklymak
Copy link
Copy Markdown
Member Author

jklymak commented Aug 13, 2020

I hate to hold up 3.3.1 for this, but its a pretty bad regression. Feel free to re-milestone if you don't agree...

@jklymak jklymak added this to the v3.3.1 milestone Aug 13, 2020
@jklymak jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Aug 13, 2020
@jklymak
Copy link
Copy Markdown
Member Author

jklymak commented Aug 13, 2020

BTW, I guess we can argue about the usefulness of categoricals with hist, but...

@tacaswell
Copy link
Copy Markdown
Member

tacaswell commented Aug 13, 2020

I'm investigating the OSX failures, they seem to be transient...

@QuLogic QuLogic mentioned this pull request Aug 13, 2020
Closed
@QuLogic QuLogic merged commit 17a0ff7 into matplotlib:master Aug 13, 2020
@lumberbot-app
Copy link
Copy Markdown

lumberbot-app Bot commented Aug 13, 2020

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout v3.3.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 17a0ff73282c1d8699abd29670b2d42505393c45
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #18243: Fix reshape list of strings'
  1. Push to a named branch :
git push YOURFORK v3.3.x:auto-backport-of-pr-18243-on-v3.3.x
  1. Create a PR against branch v3.3.x, I would have named this PR:

"Backport PR #18243 on branch v3.3.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Aug 13, 2020

I'll add this to #18247.

QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Aug 13, 2020
@QuLogic
Backport PR matplotlib#18243: Fix reshape list of strings
bcbabb3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tacaswell tacaswell tacaswell approved these changes

@QuLogic QuLogic QuLogic approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: categorical topic: testing

Projects

None yet

Milestone

v3.3.1

Development

Successfully merging this pull request may close these issues.

_reshape_2D function behavior changed, breaks hist for some cases in 3.3.0

4 participants

@jklymak @tacaswell @QuLogic @timhoffm