- Notifications
You must be signed in to change notification settings - Fork768
Implicit float conversion in function calls#1908
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
lostmsu commentedAug 14, 2022
Implicit casts to float were disabled intentionally. For instance, |
filmor commentedAug 15, 2022
The (expected) order here is:
This should lead to a sensible overload resolution. |
lostmsu commentedAug 16, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This can never happen with a custom Python type. Also, we do not really perform overload resolution prioritization like that. E.g. the |
0230e58 to159e0ddComparefilmor commentedSep 14, 2022
Can you please provide a test-case that you expect to fail with this PR? I'm fine with working on the overload resolution, but I think making |
lostmsu commentedSep 14, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I described the test case above.
See it pick the overload based on the weather. This is because we actually do not perform any overload resolution, but this behavior prevents user from forcing exact type match with codecs when the type on Python side has If somebody wants this behavior, they can register a codec for types implementing What we might want is for |
882ccb6 toc4325ddComparelostmsu commentedSep 19, 2022
@filmor I think we still shouldnot just enable |
filmor commentedSep 19, 2022
That effectively boils down to just using the "real" |
filmor commentedSep 19, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Just to recap: This will now check before attempting a conversion whether the object identifies as a |
0b05129 to8799f82Comparefilmor commentedSep 19, 2022
@lostmsu If this is mergeable for you, I think that would solve the reported issue sufficiently that we can continue with the releases without having to touch method resolution right now (which I also really don't want to). PyTorch's |
Uh oh!
There was an error while loading.Please reload this page.
lostmsu commentedSep 19, 2022
My problem with this approach is that it does not allow us to do: stringFoo(floatx)=>"float32";stringFoo(doublex)=>"float64"; Foo(numpy.float32(42.0)) |
filmor commentedSep 19, 2022
It does, you just need to be explicit. But I get your point, I just think that the reasonable default is more important than optimising this overload. We can document this particular issue. |
rzindler commentedSep 19, 2022
Thanks guys! |
lostmsu commentedSep 19, 2022
I think we can do better than document, and actually special case ctypes, NumPy, PyTorch, and TensorFlow types that have exact precision. Preferably also leave a room for users to special case other precise types. It is not that hard, and if we don't do it now we'd have to wait until 4.0 to make another breaking change. |
filmor commentedSep 19, 2022
I'm strongly against special-casing particular library types. If you want to allow proper |
8799f82 to2194d6cComparelostmsu commentedSep 19, 2022
That's exactly what I want to do in a week or two. |
filmor commentedSep 20, 2022
I'll merge this in the meantime. Feel free to revert or adjust the behaviour when you are attempting your refactoring. In this state, I'd be fine to publish it. |
What does this implement/fix? Explain your changes.
Uses
__float__when available if a Python object is passed to a function with aDoubleorSingleparameter.Does this close any currently open issues?
#1833
Any other comments?
This is not yet supported for integer types, which would use
__int__or
__index__(for Python >=3.10), as the respective logic is for somereason only implemented for
AsLongLongandAsLong, not for theunsigned counterparts and also not for the
AsSize_tvariants that weare using.
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG