-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add float type caster and revert type hint changes to int_ and float_ #5839
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
These two types do not support casting from int-like and float-like types.
I feel like Edit: After debugging this a little |
The default py::object caster only works if the object is an instance of the type. py::float_ should accept python int objects as well as float. This caster will pass through float as usual and cast int to float. The caster handles the type name so the custom one is not required.
I don't know what has happend with those tests but I feel it is beyond the scope of the changes I have made. |
Interesting |
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.
Looks good to me. @InvincibleRMC could you please also review & formally approve? I'll merge this PR after I see your approval.
tests/test_pytypes.cpp
Outdated
m.def("get_tuple_from_iterable", [](const py::iterable &iter) { return py::tuple(iter); }); | ||
// test_float | ||
m.def("get_float", [] { return py::float_(0.0f); }); | ||
m.def("cast_float", [](py::float_ f) { return f; }); |
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.
Could you please rename this to roundtrip_float
or float_roundtrip
or similar?
(cast
is super ambiguous in the context pybind11; C/C++ cast, to-Python, from-Python, all in the same name bucket unfortunately)
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.
done
@rwgk do you have any idea what is causing the tests to fail? These are the failing tests |
@msimacek Could you please take a look at these graalpy failures? We believe they are unrelated to this PR. https://github.com/pybind/pybind11/actions/runs/17764708439/job/50485236713 |
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.
Looks good to me. Nice fix.
I'm using PR #3939 to see if current |
Looks like it only ran half the tests and not the ones that failed here. |
Yeah :-( |
I just manually cancelled the workflow, to not block those two runner slots for several more hours until the tests hit the timeout. |
CI passes under #3939, but I'm a bit confused at the minute TBH:
So that's 79 total vs 85 I need to look more carefully, probably I'll get a chance only tomorrow or so. If there is anything you see, please let me know. @henryiii JIC that's something obvious to you |
The tests that failed here have passed in your PR. It must be something I changed but I have no idea what it is. The output is the same up to this line
This is the first test that did not run
Are your tests run sequentially? Is this where it is failing? The only potential break I can see is the removal of |
I will try running the tests tomorrow in my branch and see where it breaks |
Sorry I'm not sure I understand the question. I did not run anything locally, or change anything in PR #3939, except changing one character in the main README. I.e. all tests should run identically to your PR. But then again, there is the difference in the total number of tests that's still a mystery to me. |
Trying another super quick and easy thing: PR #5850 Just started. But now I see already that the total number of tests is 85, like here. |
All 83 tests pass under #5850: it seems very certain now that there is a problem with this PR, unfortunately, or maybe this PR is triggering a latent bug in graalpy. @gentlegiantJGC I think it could save you time (potentially a lot) if you wait until we hear back from @msimacek. |
It was a reference counting mistake I had made. Another test is now stuck indefinetly. No error for this one though. You may want to add a more sensible maximum time limit for workflows. |
I think I have seen that one a few times before, most likely it's unrelated to this PR. I just cancelled the job and then triggered a rerun.
It'd be awesome if you could send a PR for that. 90 minutes will probably never time out unless there is an actual bug. |
Ah, I missed that there was a |
Description
The casting rules and type hints for
py::float_
andpy::int_
should match the python type hint conventions.An input typed with
int
can only acceptint
types andfloat
can acceptfloat
orint
types as defined in as per PEP 484.when an argument is annotated as having type float, an argument of type int is acceptable
Expected behaviour
Current behavour
The type hints were changed in #5540 by @InvincibleRMC but the classes do not support these casting rules.
Fixes #5840
Suggested changelog entry: