API: Allow comparisons with and between any python integers #24915
Conversation
There was a problem hiding this comment.
This looks very nice! Quite a few inline comments, but most are small (partially about removing some strange references to "string" ufuncs).
Larger is whether the "raw" version should not just get to deal with everything - just pass in all information and let it deal with it as it pleases (possibly leaving the casting check, but not worrying about interpreting the inputs, setting original descriptors, etc.).
p.s. Thanks for splitting this of, it is much easier to wrap one's head around smaller pieces...
| int overflow; | ||
| long long val = PyLong_AsLongLongAndOverflow(value, &overflow); | ||
| if (val == -1 && overflow == 0 && PyErr_Occurred()) { | ||
| return (NPY_CASTING)-1; |
There was a problem hiding this comment.
This is confusing - are you recasting? To what? Would seem worth a comment!
There was a problem hiding this comment.
Ah, I added the -1 for "error" to the casting enum at some point, but always with an underscore and just returned -1 everywhere for error. Turns out C++ dosn't like just returning -1, so I need to either write _NPY_ERROR_OCCURRED_IN_CAST or add the cast...
There was a problem hiding this comment.
I think _NPY_ERROR_OCCURRED_IN_CAST is clear; might as well use it for new code.
| int arr_idx = 0; | ||
| int scalar_idx = 1; | ||
|
|
||
| if (dtypes[0] == &PyArray_PyIntAbstractDType) { |
There was a problem hiding this comment.
Nit, but I'd write
npy_bool first_is_pyint = dtypes[0] == &PyArray_PyIntAbstractDType;
int arr_idx = first_is_pyint? 1: 0;
int scalar_idx = first_is_pyint? 0: 1; # or 1-arr_idx
PyObject *scalar = input_scalars[scalar_idx];
and then use first_is_pyint and scalar below.
| if (input_scalars[scalar_idx] != NULL | ||
| && PyLong_CheckExact(input_scalars[scalar_idx])) { | ||
| value_range = get_value_range(input_scalars[scalar_idx], arr_dtype->type_num); | ||
| if (value_range == -2) { |
There was a problem hiding this comment.
Is -2 just the same as (NPY_CASTING)-1? If so, I'd made that explicit, i.e., if (value_range == (NPY_CASTING)-2)
There was a problem hiding this comment.
I will change it to if (get_value_range(..., &value_range) < 0) {return...}, they are not the same, just translating one error to another, but its OK to be explicit (and it should get inlined anyway).
| * function supports *both* directions for all types. | ||
| */ | ||
| static NPY_CASTING | ||
| resolve_descriptors_raw( |
There was a problem hiding this comment.
This name is rather bad. Just resolve_descriptors_pyscalars?
There was a problem hiding this comment.
Thanks, never liked it either! just didn't have a good idea. Maybe just _scalars or even _with_scalars? Because I think it may be nice to allow that in some future.
| if (value_range == -2) { | ||
| return (NPY_CASTING)-1; | ||
| } | ||
| if (arr_idx == 1) { |
There was a problem hiding this comment.
This would be if (first_is_pyint)
| /* NOTE: This should receive global symbols? */ | ||
| PyArray_DTypeMeta *Bool = PyArray_DTypeFromTypeNum(NPY_BOOL); | ||
|
|
||
| /* We start with the string loops: */ |
| promoter = PyCapsule_New( | ||
| (void *)&pyint_comparison_promoter, "numpy._ufunc_promoter", NULL); | ||
| if (promoter == NULL) { | ||
| Py_DECREF(promoter); |
There was a problem hiding this comment.
Don't decref NULL - dtype_tuple?
| goto finish; | ||
| } | ||
|
|
||
| /* All String loops */ |
There was a problem hiding this comment.
Sorry, I had started with a string ufuncs as they are also templated in C++ for all comparisons, and wasn't careful enough with removing all th leftovers. Will look through more carefully.
numpy/core/src/umath/ufunc_object.c
Outdated
| @@ -4475,14 +4476,60 @@ _get_fixed_signature(PyUFuncObject *ufunc, | |||
| * need to "cast" to string first). | |||
| */ | |||
| static int | |||
| resolve_descriptors(int nop, | |||
| resolve_descriptors(int nop, int nin, | |||
There was a problem hiding this comment.
Is it useful to pass in nin? I though that one could just get that with ufunc->nin? (Similarly, is nop necessary?)
There was a problem hiding this comment.
More generally, I wonder if it wouldn't make more sense to just pass through everything one could possibly want to the _raw version. If a ufunc wants raw, perhaps it should deal with the nitty-gritty (including even check_safety, or set that up differently so one is not stuck with original_dtypes). In that case, the name is more appropriate too...
The question then is if there is anything missing from the current argument list above that could possibly be useful? Logically, I guess it is just the outputs -- which could be easily attached by passing through full_args rather than just full_args.in.
If so, I'd suggest to remove nin and add ufunc_full_args full_args
There was a problem hiding this comment.
Yes we don't have to pass nin and nout, it is on the ufunc and ufuncimpl. I guess for a single nop it seemed easier, with both it is getting unclear.
Right, I would be happy to add outputs in principle. Right now I did this because I limited to true scalars and those are simply never outputs (i.e. the i < nout is only a sanity check).
One reason was that it isn't necessary trivial to distinguish a scalar from a 0-D object and the original 0-D object may need to be converted to an array to do anything with it so at that point I would have to pass both if you were to actually use that information reasonably. One thing that could work is checking for whether it is a scalar based on the DType, even when not abstract, and otherwise pass the array.)
I havne't thought of integrating the cast-safety check. Right now we have a "disable cast safety" special path for logical ufuncs loop (casting to bool is always OK).
I would be happy with passing the cast-safety and either allowing or forcing you to take care of it. I.e. we can remove the flag or if you want to deal with it you use set the flag so NumPy doesn't check again.
(I am not aware of where it would be useful to side-step the check except for that logical bool, bool -> bool loop.)
I would agree that there might also be a good reason to allow a "let me do everything" path where we let the user do as much as reasonable (e.g. also the full iteration). I think we should have that although finding the right API for it might not be trivial (unless we launch back into/through effectively python?!).
It isn't super easy to get feature parity, but would allow a lot of weirder stuff and speed experimentation that might be cool. (Also just making allowing more things to look like a ufunc and use the protocol.)
There was a problem hiding this comment.
Yes, it is tricky. And indeed, if one wants more control, it would probably also be necessary to know not just the ufunc but also whether it is a call, at, reduce, etc. At which point it is no longer obvious this is the right place. Still, my tendency would be to add full_args instead of the inputs and just pass everything on, i.e., let the implementation deal with input_scalars (and original_types). But perhaps keep the check on cast safety, since that is what the function has to return anyway.
There was a problem hiding this comment.
Well, I prefer passing everything on as PyObject *const * (I will add the const) rather than two tuples. Just because I think that is the more convenient API usually, and I also think changing the way we do this in the ufunc code itself would be an improvement probably (create the tuple when we need it, we don't need it often).
Passing the method additionally, would seem OK to me. I am not sure it is useful/needed, but I could imagine it can be (if just for printing a nicer custom error message).
My uncertainty is how implementations should deal with weird types (non-arrays but also not obvious scalars), so I am not quite sure what is the best thing to pass on there if that is what they want.
Thus, my maybe silly inclination to punt on it, but maybe rename the argument. We could pass all arguments as it is, we currently just keep them NULL when it isn't quite clear what is best.
There was a problem hiding this comment.
OK, true, and I don't want to make too big a deal of it.
But I think my suggestion, passing on the outputs as well, remains easier to explain and work with. And while you are right that passing in the pre-conversion-to-array input may be quite useless from an ndarray perspective, who knows, maybe it is really handy from the perspective of a more random (sub)class that overrides __array_ufunc__ and needs some strange dtype resolution as well. (For quantities, like @ngoldbaum, we had this nice procedure of the final unit of an operation only depending on the units of the arguments, which works for all but np.power. For that case, the scalar value is all that is needed, but maybe for other cases one needs the class shape or so.)
Or, to turn the argument around, how does passing on all inputs and outputs hurt the current use cases?
There was a problem hiding this comment.
🤷 Yes. I am fine if you wish to say that I largely don't like it because I don't want you to do things like np.emath (inspect values to guess result dtype). But even after you might convince me that there are actual use-cases where this would be useful – I am sure there are –, I am confident that doing it this way will still be a bad protocol for reasons beyond being brittle for alternate array-objects and conceptually not really extending to non-arrays at all.
One extension, we could also consider is pivoting to is passing char *const *scalar_vals which would hold the raw (possibly unaligned?) original dtype data pointer (whatever that is) or the Python objects for the current Python scalars.
If we talk about shapes, what I think you actually want is passing the core dimensions (reduction size/shape), not original objects (you can't really infer much from there), this is even more interesting in the get_loop() (where we have the data available in principle).
(In a sense, a lot of this to me comes back to: Yes, we probably want a "let me do everything", or even an alternative pivot where we allow more ufunc-like objects to dispatch via __array_ufunc__ to embrace a split of "ufunc as an implementation" and "ufunc as a protocol" a bit.)
There was a problem hiding this comment.
My problem remains that the current approach feels super directed just at the particular use case. If that was done privately, without opening API for it, that would be perfectly fine. But if one adds new API, it would seem nice to be not so directed at one particular use case.
But maybe your idea for the extension is a way to resolve this. Though would one perhaps generalize it slightly? Either the original dtype or the object if it had no dtype (i.e., getattr(obj, "dtype", obj)). Or maybe just anything that did not have a dtype, and NULL otherwise? (I'm not quite sure any more what processing has happened to the dtype before we get to this stage; maybe passing the original dtype is best if it can be different.)
There was a problem hiding this comment.
Yes, it is directed at the string multiplication/unit power use-case, because that is the biggest hole I am aware off and there is some overlap in the solution.
Maybe the main problem is that it isn't quite clear yet what the correct pattern is of enabling that use case.
I can live with getting it wrong (i.e. deprecating it later). But we really need a solution for NEP 50's sake more than anything. So I am OK either way:
- Just pass all arguments and make it public. I dislike it, so I would add a scary warning to it (maybe an underscore), but in the end we are all grown-ups here after all.
- Just make it private for now.
- We iterate once and maybe land on the
char **solution (may make this use-case slightly more complex, but generalize the string mul/unit power one maybe).
Just FYI... Probably clear, but let me summarize what happens right now:
__array_ufunc__, this is the only place where you can avoid any form of array conversion.- Array coercion. This is a two step process:
- Finding shape and dtype. Which includes calling
__array__()when necessary. - If necessary, creating a new array and filling it in.
Especially for scalars, we don't have to do the second step in principle. But for most array-like objects, we currently don't allow delaying the full conversion anyway.
- Finding shape and dtype. Which includes calling
- Promotion based on the DType classes.
- This find the
ArrayMethod
- This find the
- The resolution step (this one). We clearly have the "original" dtype instances available here. (For Python integers, this right now means its could be an
int64,uint64orobjectdtype).- Normally, we will convert/cast these to the actual loop DType class so that the user doesn't have to worry about that. (This is normally a de-facto no-op, because most dtypes are non-parametric)
My problem remains that the current approach feels super directed just at the particular use case.
Yes, it clearly is directly at solving exactly two things. First, side-stepping the preparation because in its current form it fails for abstract DTypes and, second, trying to create/prepare a path forward for unit power and string multiply. (maybe it isn't particularly good at that, because you may want to support 3-4 things: unit**4, unit**np.uint8(4) and unit**np.array([4]); where the last IMO should include array-likes if you allow it).
There was a problem hiding this comment.
I can't think of a clear solution to this... So, I made the slot raise an error when used as public API (effectively making it private).
numpy/core/tests/test_half.py
Outdated
| @@ -266,17 +266,17 @@ def test_half_correctness(self): | |||
| if len(a32_fail) != 0: | |||
| bad_index = a32_fail[0] | |||
| assert_equal(self.finite_f32, a_manual, | |||
| "First non-equal is half value %x -> %g != %g" % | |||
| (self.finite_f16[bad_index], | |||
| "First non-equal is half value 0x%x -> %g != %g" % | |||
There was a problem hiding this comment.
Uhh, yeah, the test had failed on me when I had a buggy version which caused this to fail cryptically...
EDIT: Left it for now, because its so small...
This implements comparisons between NumPy integer arrays and arbitrary valued
Python integers when weak promotion is enabled.
To achieve this:
* I allow abstract DTypes (with small bug fixes) to register as loops (`ArrayMethods`).
This is fine, you just need to take more care.
It does muddy the waters between promotion and not a bit if the
result DType would also be abstract.
(For the specific case it doesn't, but in general it does.)
* A new `resolve_descriptors_raw` function, which does the same job as
`resolve_descriptors` but I pass it this scalar argument
(can be expanded, but starting small).
* This only happens *when available*, so there are some niche paths were this cannot
be used (`ufunc.at` and the explicit resolution function right now),
we can deal with those by keeping the previous rules (things will just raise
trying to convert).
* The function also gets the actual `arrays.dtype` instances while I normally ensure that
we pass in dtypes already cast to the correct DType class.
(The reason is that we don't define how to cast the abstract DTypes as of now,
and even if we did, it would not be what we need unless the dtype instance actually had
the value information.)
* There are new loops added (for combinations!), which:
* Use the new `resolve_descriptors_raw` (a single function dealing with everything)
* Return the current legacy loop when that makes sense.
* Return an always true/false loop when that makes sense.
* To achieve this, they employ a hack/trick: `get_loop()` needs to know the value,
but only `resolve_descriptors_raw()` does right now, so this is encoded on whether we use
the `np.dtype("object")` singleton or a fresh instance!
(Yes, probably ugly, but avoids channeling things to more places.)
Additionally, there is a promoter to say that Python integer comparisons can just use
`object` dtype (in theory weird if the input then wasn't a Python int,
but that is breaking promises).
* Smaller cleanups/fixes * Rename the function to `resolve_descriptors_with_scalars` for now * Rename the file to `special_integer_comparisons` since that is clearer
Not sure when this got lost, but should be easy to deal with any conflicts
cdfa82c to
df219d0
Compare
Makign private by raising an error. Maybe a bit odd, but OTOH, it does advertise that we can make it public quite easily if we want to.
| /* We may want to adapt the `get_loop` signature a bit: */ | ||
| #define _NPY_METH_get_loop 2 | ||
| #define NPY_METH_get_reduction_initial 3 | ||
| #define _NPY_METH_get_loop 4 |
| "slot private due to uncertainty about the right " | ||
| "signature (see gh-24915)"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
I'm fine with coming back to this when someone has a real need for it.
There was a problem hiding this comment.
Same here, fine to come back to this.
There was a problem hiding this comment.
Not adding, I can't see a comment that does more than repeat the error message. Added a leading underscore to the slot ID, though, so it is less awkward to keep it in a public header.
| /* | ||
| * Rarely needed, slightly more powerful version of `resolve_descriptors`. | ||
| * See also `resolve_descriptors_function` for details on shared arguments. | ||
| */ |
There was a problem hiding this comment.
Would be good to add a comment here that this one is private for now given uncertainties about the signature.
| "slot private due to uncertainty about the right " | ||
| "signature (see gh-24915)"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Same here, fine to come back to this.
numpy/_core/src/umath/ufunc_object.c
Outdated
| @@ -115,10 +115,11 @@ static PyObject * | |||
| prepare_input_arguments_for_outer(PyObject *args, PyUFuncObject *ufunc); | |||
|
|
|||
| static int | |||
| resolve_descriptors(int nop, | |||
| resolve_descriptors(int nop, int nin, | |||
There was a problem hiding this comment.
Hmm, I think I'd still remove this extra addition of nin, since you only need it for the private case: just use ufunc->nin there. (Of course, no choice for inputs_tup!)
numpy/_core/src/umath/ufunc_object.c
Outdated
| @@ -2803,8 +2804,8 @@ reducelike_promote_and_resolve(PyUFuncObject *ufunc, | |||
| * casting safety could in principle be set to the default same-kind. | |||
| * (although this should possibly happen through a deprecation) | |||
| */ | |||
| if (resolve_descriptors(3, ufunc, ufuncimpl, | |||
| ops, out_descrs, signature, casting) < 0) { | |||
| if (resolve_descriptors(3, 2, ufunc, ufuncimpl, | |||
numpy/_core/src/umath/ufunc_object.c
Outdated
| @@ -4475,14 +4476,60 @@ _get_fixed_signature(PyUFuncObject *ufunc, | |||
| * need to "cast" to string first). | |||
| */ | |||
| static int | |||
| resolve_descriptors(int nop, | |||
| resolve_descriptors(int nop, int nin, | |||
| original_dtypes[i] = PyArray_DTYPE(operands[i]); | ||
| Py_INCREF(original_dtypes[i]); | ||
| } | ||
| if (i < nin |
There was a problem hiding this comment.
And change to ufunc->nin here.
numpy/_core/src/umath/ufunc_object.c
Outdated
| @@ -4856,8 +4905,8 @@ ufunc_generic_fastcall(PyUFuncObject *ufunc, | |||
| } | |||
|
|
|||
| /* Find the correct descriptors for the operation */ | |||
| if (resolve_descriptors(nop, ufunc, ufuncimpl, | |||
| operands, operation_descrs, signature, casting) < 0) { | |||
| if (resolve_descriptors(nop, nin, ufunc, ufuncimpl, | |||
numpy/_core/src/umath/ufunc_object.c
Outdated
| @@ -6228,8 +6277,8 @@ ufunc_at(PyUFuncObject *ufunc, PyObject *args) | |||
| } | |||
|
|
|||
| /* Find the correct operation_descrs for the operation */ | |||
| int resolve_result = resolve_descriptors(nop, ufunc, ufuncimpl, | |||
| tmp_operands, operation_descrs, signature, NPY_UNSAFE_CASTING); | |||
| int resolve_result = resolve_descriptors(nop, ufunc->nin, ufunc, ufuncimpl, | |||
numpy/_core/src/umath/ufunc_object.c
Outdated
| @@ -6557,8 +6606,9 @@ py_resolve_dtypes_generic(PyUFuncObject *ufunc, npy_bool return_context, | |||
| } | |||
|
|
|||
| /* Find the correct descriptors for the operation */ | |||
| if (resolve_descriptors(ufunc->nargs, ufunc, ufuncimpl, | |||
| dummy_arrays, operation_descrs, signature, casting) < 0) { | |||
| if (resolve_descriptors(ufunc->nargs, ufunc->nin, ufunc, ufuncimpl, | |||
9ce67f6 to
7c70ad2
Compare
|
Thanks @seberg! Let's pull this in to unblock NEP 50 progress. |
|
Thanks, both, this is really nice, and good that comparisons now work sensibly!! |
This implements comparisons between NumPy integer arrays and arbitrary valued
Python integers when weak promotion is enabled.
To achieve this:
ArrayMethods).This is fine, you just need to take more care.
It does muddy the waters between promotion and not a bit if the
result DType would also be abstract.
(For the specific case it doesn't, but in general it does.)
resolve_descriptors_rawfunction, which does the same job asresolve_descriptorsbut I pass it this scalar argument(can be expanded, but starting small).
be used (
ufunc.atand the explicit resolution function right now),we can deal with those by keeping the previous rules (things will just raise
trying to convert).
arrays.dtypeinstances while I normally ensure thatwe pass in dtypes already cast to the correct DType class.
(The reason is that we don't define how to cast the abstract DTypes as of now,
and even if we did, it would not be what we need unless the dtype instance actually had
the value information.)
resolve_descriptors_raw(a single function dealing with everything)get_loop()needs to know the value,but only
resolve_descriptors_raw()does right now, so this is encoded on whether we usethe
np.dtype("object")singleton or a fresh instance!(Yes, probably ugly, but avoids channeling things to more places.)
Additionally, there is a promoter to say that Python integer comparisons can just use
objectdtype (in theory weird if the input then wasn't a Python int,but that is breaking promises).
This is the split out from gh-23912, to solve the biggest pain point/problem. Comparisons with Python integers that are out-of-bounds.
May need a bit of cleanup, but the main discussion should be if the appraoch is really what we want (but we really need something).