-
-
Notifications
You must be signed in to change notification settings - Fork 8k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
This is numpy and pytorch making different choices about what to do when cast to a Python scalar
In [1]: import numpy as np In [2]: float(np.array([[1, 2], [3, 5]])) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) Cell In[2], line 1 ----> 1 float(np.array([[1, 2], [3, 5]])) TypeError: only length-1 arrays can be converted to Python scalars
...d others) plotting
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.x Backport PR #26807 on branch v3.8.x (Catch ValueError to support pytorch (and others) plotting)
OTOH, pytorch should probably return an error consistent with numpy
yeah, but then they have a changed-behavior and deprecation problem. Guess they could raise a multiply inherited exception from both TypeError and ValueError 😈
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