BUG: Add promoter to `ldexp` for python integers to prevent overflow by MaanasArora · Pull Request #29597 · numpy/numpy

MaanasArora · 2025-08-20T03:01:34Z

Adds a promoter for first argument PyArray_PyLongDType to ldexp. It casts straight to double, not sure if that is more arbitrary than needed.

Thanks for reviewing!

mattip · 2025-08-20T09:31:56Z

What will happen with complex input?

MaanasArora · 2025-08-20T17:11:29Z

@mattip this only promotes integer inputs (Python scalar integer, actually), so the rest would stay the same?

seberg · 2025-08-26T12:45:55Z

Thanks a for looking at this, this is very cool to see this use!

I am curious if we can push this further and actually use the promoter always. That is, we:

Set the first and last dtype to PyArray_CommonDType(first_dtype, AbstractFloatDType):
- If the last was forced, we just set the first to that instead.
- Yes, we ignore the second (integer) argument here considering how it is currently defined.
Set the second dtype to PyArray_CommonDType(second_dtype, int_dtype). That just seems like it always works in practice, so I don't really feel a need to do something more complicated.

MaanasArora · 2025-08-27T04:48:23Z

Thanks, that extension makes a lot of sense!

I tried implementing this, but it causes a segmentation fault. It seems to be due to the fact that common_dtype is not defined on FloatPyArray_FloatAbstractDType (in the dtypemeta declaration). I'm wondering if the abstract classes support the common dtype operation yet? (I assume the python integers won't be the ones casting up to floats)

Edit: that does seem to be it; reverting the operation for the first dtype and setting the others in this way doesn't crash (though of course the promotion doesn't work properly)

seberg · 2025-08-27T06:12:59Z

Oh, hmmm, the pyfloat one should work ok probably, although the abstract version woukd be nicer.

MaanasArora · 2025-08-27T22:03:03Z

PyFloat doesn't seem to work well oddly, but Float does. I've pushed that, but yeah it's not as nice. It could be interesting to look into supporting common_dtype more fully for the abstract types if you'd like!

seberg · 2025-08-28T06:32:49Z

Making the abstract and PyFloat do the same thing probably makes sense, but the fact that it doesn't work here seems like a bug somewhere that I (or we) need track down to make this work (and a couple of other cases, because we should use this for other float only functions).

MaanasArora · 2025-08-29T03:50:20Z

To be clear it doesn't crash, it just doesn't promote further up than float16 so causes overflow.

In line 223-226 of abstractdtypes.c we do in float_common_dtype:

    else if (other == &PyArray_PyLongDType) {
        Py_INCREF(cls);
        return cls;
    }

This is equivalent to doing no promotion as we already know it's a float, but looking at NEP 50, this actually seems correct behavior - Python ints can promote to the lowest float (given we need a float) right? So for ldexp if we don't consider argument values, we would have to just use something that can fit all values?

MaanasArora · 2025-08-29T04:06:14Z

Yes, we promote to DoubleDType when the other dtype is any legacy integer, but do not promote (keep the same class) when it is PyLong. That seems a bit inconsistent, so it is probably buggy in at least one case? Unless I'm missing something

numpy/numpy/_core/src/multiarray/abstractdtypes.c

Lines 217 to 226 in 583993f

    
           if (NPY_DT_is_legacy(other) && other->type_num < NPY_NTYPES_LEGACY) { 
        
               if (other->type_num == NPY_BOOL || PyTypeNum_ISINTEGER(other->type_num)) { 
        
                   /* Use the default integer for bools and ints: */ 
        
                   return NPY_DT_NewRef(&PyArray_DoubleDType); 
        
               } 
        
           } 
        
           else if (other == &PyArray_PyLongDType) { 
        
               Py_INCREF(cls); 
        
               return cls; 
        
           }

seberg · 2025-08-29T05:25:48Z

Ah, that makes sense (if I am getting the right picture). We are getting the abstract version as a result. In other paths that is OK because we only need a concrete dtype instance (via default_descr), but here we need to convert the abstract one to the concrete Float64 one.
I actually don't think I needed that before, it seems like a missing piece for generic promotion!
We could do it via the default_descr type, which not be so bad in practice, although it's reversed...

MaanasArora · 2025-09-01T02:19:01Z

Ah, I see, thanks! Yes, I think that's right, we're getting the PyFloat back, which is abstract. Should common_dtype not return abstract dtypes (i.e., are dtypes concretized there)? Then yes, we could do default_descr if the dtype is abstract. I'm not sure it being reversed is a concern because we often rely on symmetry in these casts?

seberg · 2025-09-01T07:24:39Z

Yeah, I suppose it probably isn't a concern in practice, so the choice we have may be the pragmatic one, even if it is slightly weird/reversed if we use it here. Reverse or not, until we have a case where this fails, it may be a reasonable to path to "promote to a concrete DType".

MaanasArora · 2025-09-01T08:27:20Z

Makes sense! If it's a missing step, I suppose we should add it to PyArray_CommonDType unless that's too big and we'd rather have it localized? Just pushed it there! I did notice though, the python types (like PyFloat) don't seem to have the abstract flag, while tp_base is set to the abstract version. I'm just checking if default_descr exists but not sure if that's ideal.

(I realize this is an unrelated change to this PR, so completely happy to revert and open another PR if you prefer.)

seberg · 2025-09-01T08:40:20Z

I suppose we should add it to PyArray_CommonDType unless that's too big and we'd rather have it localized?

Yeah, I am not sure I want that function to ensure no abstract result. We could add a mini internal helper for now that ensures we have a concrete dtype, also means we can refactor it in a single place if we ever have to.
We should check NPY_DT_is_abstract before doing this dance, there is a NPY_DT_CALL_default_descr macro and we ensure that it cannot be NULL (although it could fail, but that is probably OK).

(Also could check if you need the PyFloat rather than the abstract float version, since that didn't actually help initially.)

MaanasArora · 2025-09-01T08:48:38Z

Makes sense! I actually meant that NPY_DT_is_abstract didn't work for some reason. It doesn't seem we initiate the PyFloat with NPY_DT_ABSTRACT, though we do it for AbstractDType. If we want it to be abstract should we add the flag? (Or is it inherited from tp_base? It doesn't seem to be)

Using the AbstractDType would be a better call probably, but since common_dtype is undefined, doing that still causes a segmentation fault. (I can define the common dtype if you like, but should probably do it for all abstract dtypes then.)

seberg · 2025-09-01T08:53:57Z

It doesn't seem we initiate the PyFloat with NPY_DT_ABSTRACT, though we do it for AbstractDType.

Hmmmmmmm, we had some reason for changing that. FWIW, it's probably fine then, we should be using the abstract version.
The common_dtype for the abstract version should be defined identically, but not sure it would be easy to not just duplicate the code (which is probably fine).

MaanasArora · 2025-09-01T08:55:35Z

We can just pass the float_common_dtype pointer right? There are no dt slots defined for the abstract dtypes as far as I can tell, but we could create that struct and then pass common_dtype = float_common_dtype?

Edit: unless there is a reason to not define the slots (I guess not, since the only other two are default_descr and discover_descr_from_pyobject, which would not be needed, so coincidence?)

seberg · 2025-09-01T08:57:18Z

We can just pass the float_common_dtype pointer right?

Basically except that may return one of the Py*DType which would be wrong when we need to return one of the Abstract*DType.

MaanasArora · 2025-09-01T09:06:20Z

Hmm, there doesn't seem to be a reference to the Py*DType at least in the float version! So it should probably be fine.

I've added two slots to the abstract float dtype (I can add if you want them on the other abstract dtypes as well). The internal helper, should it be only for promoters?

Edit: sorry, I suppose if it's a helper it can just be used directly in ldexp? Pushed this.

MaanasArora · 2025-09-01T09:41:36Z

The new test seems to reliably fail on armhf (will investigate).

jorenham · 2025-11-18T23:48:14Z

A rebase wouldn't hurt

… promotion

seberg · 2025-11-19T09:04:33Z

Pushed a small fixup mostly for additional error checks. I also broadened it up to all integer inputs (including int32, etc.).

NumPy promotion should go to float64 for integers of any type by default for functions that do not support integers normally, I think. Since we add the machinery to do that, might as well do that now I think.

Let's see if tests pass now, although I am not sure what that armhf failure was... If they do, I think I'll merge it soon.

seberg · 2025-11-19T09:26:07Z

What will happen with complex input?

Nothing, the promoter will never be called and even if it was, it wouldn't do anything. There are no complex loops anyway.

seberg · 2025-11-19T10:42:22Z

+        Py_CLEAR(new_op_dtypes[0]);
+        return -1;
+    }
+    if (new_op_dtypes[2] == NULL) {


Note to solve (or Maanas), forgot an INCREF. Also not sure if we should check signature, although... I suppose it doesn't really matter, the promotion above should be always OK and if it causes a signature mismatch I think the later machinery would raise an error.

The test failures are all on platforms with int being 32bit integers, although I am not sure that is the underlying reason.
I somewhat suspect it may be an issue with the SIMD code, a way forward may to ignore this warning/FPE and check what the result type (and value, but if that fails just the type to see if the promotion is right).

Thanks - added the INCREF.

…ify result dtype

…result dtype

MaanasArora · 2025-11-19T11:10:06Z

Thanks for the cleanup!

I somewhat suspect it may be an issue with the SIMD code, a way forward may to ignore this warning/FPE and check what the result type (and value, but if that fails just the type to see if the promotion is right).

Yes, likely some downstream bug, since it's only on a few platforms. I've added a test to check the result dtype is np.float64 and suppress the warnings (not sure if the TODO is too much, but I guess good to be explicit there).

seberg · 2025-11-19T11:32:43Z

Yes, likely some downstream bug

Well, this is should be in the SIMD code path I think, but let's see. So it may or may not be the system math being at fault for spuriously giving a warning.

seberg · 2025-11-19T11:33:47Z

+            warnings.filterwarnings('ignore', message='overflow encountered in ldexp',
+                                    category=RuntimeWarning)
+            warnings.filterwarnings('ignore', message='overflow encountered in cast',
+                                    category=RuntimeWarning)


There may be a test not liking this. We can use pytest.mark.filterwarnings here, I think.

(But let's see what is going on first, I guess...

All tests pass now, but I can still push this if it's better!

Ah nice. I think it would be a bit nicer, since it is not relevant to the test (and removes the indentation).

Just to understand, the overflow is actually real on these platforms? I.e. they give the incorrect result?
We should open an issue for this if that is the case, as I think it indicates a bug in the SIMD code (or so).
(For the cherry on top, we add a comment that links to the issue.)

Ah, two other small things if you touch things, but I can do it also:

I guess it's now big enough that parametrization may be nice.

The dtype check would be nice to always do I think. While the result check is OK if it fails (due to an issue).

Makes sense! I've pushed a refactored test with parametrization and pytest.mark.filterwarnings.

I'm not sure if the overflow is real - I only saw the warning for sure in CI and I don't have access to one of these (armhf is the main one, IIRC). I've pushed it without the try, so let's see if it catches.

Yes quite strange! It seems the dtype is being promoted incorrectly on the failing Windows MSVC and armhf:

FAILED numpy/_core/tests/test_umath.py::TestLdexp::test_ldexp_integer_scalar[int32-int] - AssertionError: assert dtype('float16') == <class 'numpy.float64'> FAILED numpy/_core/tests/test_umath.py::TestLdexp::test_ldexp_integer_scalar[int32-int8] - AssertionError: assert dtype('float16') == <class 'numpy.float64'> FAILED numpy/_core/tests/test_umath.py::TestLdexp::test_ldexp_integer_scalar[int32-int16] - AssertionError: assert dtype('float32') == <class 'numpy.float64'>

I'm not sure why the promotion is platform dependent either. (Not to mention, it seems assert_almost_equal actually passed? That is odd, given the result is clearly overflown to infinity.)

The test failures are all on platforms with int being 32bit integers, although I am not sure that is the underlying reason.

I think this is relevant - the overflow only happens when the first integer is int32. In fact it seems we call the third branch of float_common_dtype (from inspecting the error outputs), so perhaps the first condition is skipped for some reason?

numpy/numpy/_core/src/multiarray/abstractdtypes.c

Lines 214 to 227 in 0ecf780

static PyArray_DTypeMeta *

float_common_dtype(PyArray_DTypeMeta *cls, PyArray_DTypeMeta *other)

{

if (NPY_DT_is_legacy(other) && other->type_num < NPY_NTYPES_LEGACY) {

if (other->type_num == NPY_BOOL || PyTypeNum_ISINTEGER(other->type_num)) {

/* Use the default integer for bools and ints: */

return NPY_DT_NewRef(&PyArray_DoubleDType);

}

}

else if (other == &PyArray_PyLongDType) {

Py_INCREF(cls);

return cls;

}

else if (NPY_DT_is_legacy(other)) {

Yeah, taking the last branch here could explain it, but I can't say I see a way that this branch can be taken really unless other->type_num is corrupted and I don't really see that happening either!?

As to why it passed so oddly (and why emscripten passed before), it did float16(inf) == large_number, but float16(large_number) was also inf and that gives the same warning, but if you ignore the warnings it passes.

Yeah, I don't really see how it could be taken either! The outputs are weirdly consistent with it still. Perhaps the promoter is just not being called? Also seems unlikely, as I don't see why int32 would not be under abstract int. That said, int32 does seems 'special' on these platforms.

True, but if the promoter wasn't used, I would expect it to pick the first loop that supports safe casting, and int32 -> float16 isn't safe casting. Feels like we are missing something "obvious" maybe, but maybe hard to figure out without a 32bit system.

…uppress warnings

github-actions Bot added the 00 - Bug label Aug 20, 2025

seberg added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Aug 26, 2025

MaanasArora added 7 commits November 19, 2025 09:51

BUG: Add ldexp ufunc promoter for Python integers

e43cd4c

ENH: Add test for ldexp with Python scalar inputs

f3c2846

BUG: Fix ldexp promoter to use correct operand dtypes

56c3b72

DOC: Update ldexp example output dtype to match new behavior

60ccd96

BUG: Update ldexp promoter to use common dtypes for input operands

0d6e528

DOC: Update ldexp example output dtype to match new behavior

780cf24

BUG: Concretize common abstract dtypes and update ldexp promoter

6a03205

MaanasArora and others added 5 commits November 19, 2025 09:51

DOC: Update ldexp example output dtype to float64 for consistency

6f526dd

BUG: Update ldexp promoter to use FloatAbstractDType with added dt slots

d8a5dd3

BUG: Replace concretization in CommonDType with small helper in ldexp…

9595399

… promotion

BUG: Move ensure_concrete_dtype to abstractdtypes

c5f875b

Small fix-up and broaden to NumPy integers

246adb3

seberg force-pushed the bug/ldexp-integer-promotion branch from f26c334 to 246adb3 Compare November 19, 2025 09:01

seberg reviewed Nov 19, 2025

View reviewed changes

MaanasArora added 3 commits November 19, 2025 05:52

BUG: Increment reference count for result dtype in ldexp promoter

e446c19

TST: Suppress overflow warnings in ldexp integer scalar tests and ver…

538469a

…ify result dtype

BUG: Handle assertion errors in ldexp integer scalar tests to verify …

b8babef

…result dtype

TST: Suppress overflow warnings for casts in ldexp integer scalar tests

3ddbdb5

seberg reviewed Nov 19, 2025

View reviewed changes

TST: Refactor ldexp integer scalar test to use parameterization and s…

854eade

…uppress warnings

	static PyArray_DTypeMeta *
	float_common_dtype(PyArray_DTypeMeta cls, PyArray_DTypeMeta other)
	{
	if (NPY_DT_is_legacy(other) && other->type_num < NPY_NTYPES_LEGACY) {
	if (other->type_num == NPY_BOOL \|\| PyTypeNum_ISINTEGER(other->type_num)) {
	/* Use the default integer for bools and ints: */
	return NPY_DT_NewRef(&PyArray_DoubleDType);
	}
	}
	else if (other == &PyArray_PyLongDType) {
	Py_INCREF(cls);
	return cls;
	}
	else if (NPY_DT_is_legacy(other)) {

Conversation

MaanasArora commented Aug 20, 2025

Uh oh!

mattip commented Aug 20, 2025

Uh oh!

MaanasArora commented Aug 20, 2025

Uh oh!

seberg commented Aug 26, 2025

Uh oh!

MaanasArora commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seberg commented Aug 27, 2025

Uh oh!

MaanasArora commented Aug 27, 2025

Uh oh!

seberg commented Aug 28, 2025

Uh oh!

MaanasArora commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaanasArora commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seberg commented Aug 29, 2025

Uh oh!

MaanasArora commented Sep 1, 2025

Uh oh!

seberg commented Sep 1, 2025

Uh oh!

MaanasArora commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seberg commented Sep 1, 2025

Uh oh!

MaanasArora commented Sep 1, 2025

Uh oh!

seberg commented Sep 1, 2025

Uh oh!

MaanasArora commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seberg commented Sep 1, 2025

Uh oh!

MaanasArora commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaanasArora commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorenham commented Nov 18, 2025

Uh oh!

seberg commented Nov 19, 2025

Uh oh!

seberg commented Nov 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaanasArora commented Nov 19, 2025

Uh oh!

seberg commented Nov 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaanasArora commented Aug 27, 2025 •

edited

Loading

MaanasArora commented Aug 29, 2025 •

edited

Loading

MaanasArora commented Aug 29, 2025 •

edited

Loading

MaanasArora commented Sep 1, 2025 •

edited

Loading

MaanasArora commented Sep 1, 2025 •

edited

Loading

MaanasArora commented Sep 1, 2025 •

edited

Loading

MaanasArora commented Sep 1, 2025 •

edited

Loading