Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Catch ValueError to support pytorch (and others) plotting#26807
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
jklymak left a comment
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 seems fine. For patches like this, though, it would be nice to know why it broke in the first place to be sure that the raise was unintentional.
tacaswell commentedSep 18, 2023
This is numpy and pytorch making different choices about what to do when cast to a Python scalar In [1]:importnumpyasnpIn [2]:float(np.array([[1,2], [3,5]]))---------------------------------------------------------------------------TypeErrorTraceback (mostrecentcalllast)CellIn[2],line1---->1float(np.array([[1,2], [3,5]]))TypeError:onlylength-1arrayscanbeconvertedtoPythonscalars |
…d others) plotting
jklymak commentedSep 18, 2023
This was last changed in#26303, and it indeed was made more strict there, though I think inadvertently. (Not a huge fan of micro optimizations for the reason of unintended consequences). I don't see the harm in allowing a ValueError through, OTOH, pytorch should probably return an error consistent with numpy if they want maximum compatibility. |
…807-on-v3.8.xBackport PR#26807 on branch v3.8.x (Catch ValueError to support pytorch (and others) plotting)
tacaswell commentedSep 18, 2023
yeah, but then they have a changed-behavior and deprecation problem. Guess they could raise a multiply inherited exception from both |
Uh oh!
There was an error while loading.Please reload this page.
PR summary
Closes#26806
Not sure we should add it, but will probably help other libraries as well (that we do not formally support).
No way of testing on the CI as we do not want to install torch, but I promise that it works locally. 😉
PR checklist