Skip to content

Fix for _axes.scatter() array index out of bound error #12673

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

Merged
merged 5 commits into from
Nov 2, 2018
Merged

Fix for _axes.scatter() array index out of bound error #12673

merged 5 commits into from
Nov 2, 2018

Conversation

esvhd
Copy link
Contributor

@esvhd esvhd commented Oct 30, 2018

PR Summary

This is to issue #12641, fixing the index out of bound error for accessing first element of an Iterable. Solution uses cbook.safe_first_element().

Added test case for cbook.safe_first_element() for pandas.Series object.

@ImportanceOfBeingErnest
Copy link
Member

Seems to be the same as #12672

@esvhd
Copy link
Contributor Author

esvhd commented Oct 31, 2018

#12672 at the time of writing this post still uses c[0] so would fail for pd.Series with non-0 starting index. Need to use safe_first_element() which is what this PR uses.

@jklymak jklymak added this to the v3.0.x milestone Oct 31, 2018
@jklymak
Copy link
Member

jklymak commented Oct 31, 2018

Looks like this also needs a test for the use case that was causing the original issue?

@esvhd
Copy link
Contributor Author

esvhd commented Oct 31, 2018 via email

@jklymak
Copy link
Member

jklymak commented Oct 31, 2018

You want a test that tests the scatter use cases so no one inadvertently breaks this functionaility in the future by modifying your check. The API is quite flexible (some might say too flexible) and its hard to anticipate every usecase when making changes, so we hope the tests catch them.

@esvhd
Copy link
Contributor Author

esvhd commented Nov 1, 2018

How does this look? 2 new test cases added, fixed a an easy formatting line on the way.

Borrowed the empty test case from @timhoffm.

The other test case is for the non-zero indexed pd.Series raised by @montebhoover.

@esvhd
Copy link
Contributor Author

esvhd commented Nov 1, 2018 via email

Copy link
Contributor Author

@esvhd esvhd left a comment

Choose a reason for hiding this comment

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

Added comment in test as suggested.

@phobson phobson merged commit bcb372b into matplotlib:master Nov 2, 2018
@phobson
Copy link
Member

phobson commented Nov 2, 2018

Thanks for taking this on, @esvhd

@lumberbot-app
Copy link

lumberbot-app bot commented Nov 2, 2018

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.0.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 bcb372b57367b3c6ad9973eae14a72800b97324f
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #12673: Fix for _axes.scatter() array index out of bound error'
  1. Push to a named branch :
git push YOURFORK v3.0.x:auto-backport-of-pr-12673-on-v3.0.x
  1. Create a PR against branch v3.0.x, I would have named this PR:

"Backport PR #12673 on branch v3.0.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.

@esvhd
Copy link
Contributor Author

esvhd commented Nov 2, 2018

Thanks @phobson. Is there anything I need to do about the backport comment posted by the bot? Happy to take this over the finish line.

ImportanceOfBeingErnest pushed a commit to ImportanceOfBeingErnest/matplotlib that referenced this pull request Nov 6, 2018
Fix for _axes.scatter() array index out of bound error
@ImportanceOfBeingErnest
Copy link
Member

Trying to backport this currently fails the tests for 3.5. See #12761.

tacaswell added a commit that referenced this pull request Nov 9, 2018
…of-pr-12673-on-v3.0.x

Backport PR #12673 to v3.0.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants