-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Expand float and complex strict mode to allow ints and ints/float. Updates type hints to match better with typing.SupportsIndex
#5879
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Michael Carlstrom <[email protected]>
|
I have started working on this too. |
This is known at compile time so it can be constexpr
|
Here is my implementation and documentation changes. It needs tests but I am going to stop for today. |
|
You will need to remove the constexpr check. I didn't realise it was a newer C++ feature |
Signed-off-by: Michael Carlstrom <[email protected]>
|
TODO: figure out why it doesn't work in C++11. |
Signed-off-by: Michael Carlstrom <[email protected]>
|
I have just found that a python float passed to an int argument will raise a TypeError. This is probably something we should resolve here because it effects these tests. m.def("func", [](int){});>>> func.__doc__
func(arg: typing.SupportsInt) -> None
>>> func(5.5)
TypeError
>>> func(numpy.float32(5.5)) # This works fine |
The int type caster allows anything that implements __int__ with explicit exception of the python float. I can't see any reason for this. This modifies the int casting behaviour to accept a float. If the argument is marked as noconvert() it will only accept int.
|
Here is my proposed change. You will need to update the tests. Here are my unittests |
|
I noticed something else to have address. According to the documentation all of int,float,complex support falling back on |
typing.SupportsIndex
I need to find a block of time to look at this carefully. Might be a couple days. Thanks for working on this! |
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
|
Hi @InvincibleRMC @gentlegiantJGC, I will review this PR now / this morning. QQ just in case you're around: Are there still any open questions from your side? |
|
My main question is the casting behaviour to C++ and how My understanding is that in the default (convert) mode any python type that can be converted to that type should be. The converting integral type caster accepts any object that implements |
tests: Add overload resolution test for float/int breaking change Add test_overload_resolution_float_int() to explicitly test the breaking change where int arguments now match float overloads when registered first. The existing tests verify conversion behavior (int -> float, int/float -> complex) but do not test overload resolution when both float and int overloads exist. This test fills that gap by: - Testing that float overload registered before int overload matches int(42) - Testing strict mode (noconvert) overload resolution breaking change - Testing complex overload resolution with int/float/complex overloads - Documenting the breaking change explicitly This complements existing tests which verify 'can it convert?' by testing 'which overload wins when multiple can convert?'
|
FYI — I'm playing with I'm pushing it JIC you want to follow along. There are some other things I still want to work on. I might not get all the way through today. I'll let you know here when I've arrived at a stable point, but please let me know any comments anytime. |
|
Follow up to my last message: I don't think there is a way to tell if an object is inherintly a float so I don't know how that would work for floating types. |
…rrectly with complex conversion. These should be consistent across CPython, PyPy, and GraalPy.
|
Here is how far I got today (I'll have another block of time tomorrow afternoon): To the best of my current understanding, please nudge/correct as needed (this was partially generated by cursor; hope that's OK in terms of tone): Focus of this PR: I think it'll be best to keep this PR focused on allowing Regarding For float and complex casters, this PR LGTM as-is:
The tests confirm this: The inconsistency you mentioned: The int caster is the outlier — it currently accepts Regarding type hints: The type hints for Regarding float exclusion from int caster: That's also a separate design question (related to #5889 you mentioned) that should be addressed independently. I'll see your comments, but probably won't be able to focus on this PR until tomorrow afternoon. |
|
I looked more at this PR, but still need more time. There are many changes to the tests. I'm wondering about this part:
Could we break that out as a separate PR? Would that lead to a significant reduction in the number of test changes in the main PR? |
|
I'll look into how easy it is to split them apart after work. |
Give me a moment, I'm trying to do that with cursor right now. I'm hoping that way we don't have to do the legwork ourselves. |
Description
Breaking change to allow Python ints into float when under strict typing. This change was also done to complex to allow floats and ints. Now that Python ints can be passed into floats it changes behavior for overload resolution. A method that takes float that is registered before a method that takes an int will now get executed when a Python int is passed in. This overload resolution also affects methods with
std::complex.This was done to better match PEP 484 numeric tower rules.
Add
typing.SupportsIndexto the int/float input types to better match fall backs.This corrects a mistake that these where supported but, the type hint was not updated.
Change complex input types to
typing.SupportsComplex | typing.SupportsFloat | typing.SupportsIndexto match runtime conversion rules.Resolves #5878
Provides a work around for #5767
Suggested changelog entry:
Expand float and complex strict mode to allow ints and ints/float. Updates type hints to match better with
typing.SupportsIndex.📚 Documentation preview 📚: https://pybind11--5879.org.readthedocs.build/
📚 Documentation preview 📚: https://pybind11--5879.org.readthedocs.build/
📚 Documentation preview 📚: https://pybind11--5879.org.readthedocs.build/
📚 Documentation preview 📚: https://pybind11--5879.org.readthedocs.build/
📚 Documentation preview 📚: https://pybind11--5879.org.readthedocs.build/
📚 Documentation preview 📚: https://pybind11--5879.org.readthedocs.build/
📚 Documentation preview 📚: https://pybind11--5879.org.readthedocs.build/
📚 Documentation preview 📚: https://pybind11--5879.org.readthedocs.build/
📚 Documentation preview 📚: https://pybind11--5879.org.readthedocs.build/
📚 Documentation preview 📚: https://pybind11--5879.org.readthedocs.build/
📚 Documentation preview 📚: https://pybind11--5879.org.readthedocs.build/
📚 Documentation preview 📚: https://pybind11--5879.org.readthedocs.build/