Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
FIX: let pandas IndexInt64 work for boxplot#13398
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
1e7caa0 to28bc931Comparejklymak commentedFeb 10, 2019
I think this fix is "OK" as a stopgap, but I think what really should happen is |
| positions=list(range(1,N+1)) | ||
| eliflen(positions)!=N: | ||
| raiseValueError(datashape_message.format("positions")) | ||
| positions=np.array(positions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a good fix for the case described in the ticket, but I think the code will continue raising obscure error when positions are not numbers. Can you add a check after this line about the dtype of the array being a number? Maybe something like:
ifnotisinstance(positions.dtype,numbers.Number):raiseTypeError("positions should be a iterable of dtype number, but found dtype %s"%blahblahblah.)
When we end up supporting something else than numbers, this can be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I couldn't quite get that check to work; I had to check the first element. But if there is a better way, let me know.
ef7afc4 to728106aCompareNelleV commentedFeb 19, 2019
Thanks! |
PR Summary
Closes#10022 by casting the list of positions to an array as suggested by@tacaswell
PR Checklist