-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Implicit conversions to bool + np.bool_ conversion #925
Conversation
A more general solution would be to invoke a Python API function that will call the instance's |
(the implicit conversion should only take place when |
Indeed; I forgot about py2/3 differences. Another option is to just do a
Should this really be counted as an implicit conversion? |
Right -- the question is: is there a C API binding to do exactly this.
Yes. For instance, |
|
Excellent -- that's the one. |
b29be01
to
63e57e8
Compare
Ok, a few changes:
I was wondering whether it's worth to cache |
I was thinking more along the following lines: In common.h, add: // In Python 3.x block, Line 138+
#define PYBIND11_NONZERO "__bool__"
// In Python 2.x block: Line 158+
#define PYBIND11_NONZERO "__nonzero__" Then, in the caster, use ....
else if (convert && hasattr(src, PYBIND11_NONZERO)) {
value = (bool) PyObject_IsTrue(src.ptr());
return true;
}
.... |
Note that if (convert) {
auto result = PyObject_IsTrue(src.ptr());
if (result == -1) // this *should* also cover missing `__bool__`/`__nonzero__`,
return false; // but adding a proper test to make sure would be good
value = result == 1;
return true;
} |
Oh - I've misread your previous message then. I was only thinking about Do we want to have global implicit bool conversions? (which looks a bit scary to me as most everything can be converted to a bool) If we do, then >>> hasattr([], '__bool__')
False (source for Also, for |
I am not sure that is the case:
|
Comments crossed with @dean0x7d :) Yea, if we want a generic implicit bool conversion, I'd do it like in #925 (comment) |
Also >>> bool(object())
True 🐼 |
Yeah, I overlooked that objects can sometimes be converted to
I think that going completely broad should be fine (i.e. anything that isn't an error for
def foo(arg):
if arg:
print("yes") >>> foo(object())
yes I think it's fine for pybind11 to have the same behavior, regardless of type or magic methods. Besides, it's simpler not to maintain a special pybind11-specific whitelist of what can be converted to |
Btw, looking at (... or we can just use |
My suggestion is in 5a9b757, and an example of how it works in 1a820a4. This will handle Does this look reasonable? |
93963e6
to
047a11b
Compare
Given the separate paths needed for Python 2/3 and it looks like a workaround for PyPy, I feel like the simple Tests: the ones that aren't related to numpy should be moved into |
Heh, PyPy spoilt my plans a bit, yea it looks like it needs a workaround... I personally feel like from the user's standpoint using That being said, from the dev standpoint, using // Re: tests: ditto, I'll move them before squashing |
I don't like the use of |
Does anyone have an idea on how to fix the pypy issue? :) |
PyPy's implementation of
|
include/pybind11/cast.h
Outdated
value = res != 0; | ||
return true; | ||
} | ||
} else if (hasattr(src, "dtype")) { |
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.
I think this needs to go before the else if (convert)
, otherwise it's not going to run if the arguments get into the second (convert-allowed) pass. That second pass can be triggered by some other argument that needs conversion, so anything that runs with convert = false
needs to also work with convert = true
(even if, as in this case, conversion isn't needed for this argument).
OK, I have no objections if |
Re: hasattr version -- maybe it could be done that way on PyPy for the lack of better options-- in CPython it would be much, much slower than checking for |
This still seems really complicated to me. Can’t we strip out the NumPy-specific bits (they should be handled by the default case that calls if (!src) return false;
else if (src.ptr() == Py_True) { value = true; return true; }
else if (src.ptr() == Py_False) { value = false; return true; }
else if (convert && hasattr(src, PYBIND11_NONZERO)) {
int res = PyObject_IsTrue(src.ptr());
if (res == 0 || res == 1)
return (bool) res;
}
return false; |
include/pybind11/common.h
Outdated
@@ -153,8 +153,10 @@ | |||
#define PYBIND11_SLICE_OBJECT PyObject | |||
#define PYBIND11_FROM_STRING PyUnicode_FromString | |||
#define PYBIND11_STR_TYPE ::pybind11::str | |||
#define PYBIND11_NONZERO "__bool__" |
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 we rename this to BOOL_ATTR
or something along those lines to stick to the python-3-derived name here rather than python-2-derived, like we do with the above (e.g. "BYTES").
Calling any kind of pybind11-bound function with scalar data (e.g. booleans instead of arrays of booleans) that perhaps even require implicit conversions is not going to be that fast, so I don't know useful performance tuning is at that level. |
@jagerman I've integrated your comments, but that's probably as simple as it's going to get... CPython optimization part is now just extra 4 lines of code which isn't too bad -- the rest has to be there anyway, including the none check at the start. |
include/pybind11/cast.h
Outdated
} | ||
return false; | ||
} | ||
if (hasattr(src, "dtype")) { |
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.
Does the above (i.e. the else if (convert) { ... }
block) catch a numpy bool under convert == true
? If so, this could be an else if
to save the attribute lookup during a convert
load when we already know it failed.
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.
Yes, indeed. Fixed.
By the way, I think an even faster way would be to strcmp the type name (which should cost nothing to extract) to 'bool_' and only then check the dtype, etc. But not sure it's worth it here.
include/pybind11/cast.h
Outdated
} | ||
return false; | ||
} | ||
else if (hasattr(src, "dtype")) { |
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.
What happens when this branch is completely removed? Can't we have the second (converting) pass handle NumPy booleans (which is already optimized to avoid hasattr
)?
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.
I think the intention is to get this into the no-convert pass, so that it can win the overload resolution if the function is overloaded with some other argument type (e.g. int
).
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.
Yep, correct.
// as I've noted above, I think the second hasattr()
can actually be avoided in np.bool_ cases via something cheap like strcmp(Py_TYPE(src.ptr())->tp_name, "bool_")
prior to the dtype hasattr check (and maybe the following dtype.kind
check can then be thrown out, because what else can it be but the np.bool_...)
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.
strcmp(tp_name, "bool_")
also looks like a pretty nice simplification. In that case the separate np.bool_
logic could be removed completely and just leave:
else if (convert || strcmp(tp_name, "bool_") == 0) {
...
}
a919783
to
025ba8a
Compare
Ok, I've reduced the |
That simplification is nice. Merged! |
This fixes #922 but is quite hacky / not overly efficient -- mainly because we can't use proper numpy API in
cast.h
and because bool type caster is a specialisation...(If an added if clause is a concern, I guess it could be moved over to
numpy.h
and made opt-in (e.g. add an optional function pointer intype_caster<bool>
), but then the user would have to enable it manually in their code. On the bright side, it would be more efficient, without string comparisons etc.)