Skip to content

Conversation

MaanasArora
Copy link
Contributor

Fixes #29182.

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
Copy link
Member

mattip commented Aug 20, 2025

What will happen with complex input?

@MaanasArora
Copy link
Contributor Author

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

@seberg
Copy link
Member

seberg commented Aug 26, 2025

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.

@seberg seberg added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Aug 26, 2025
@MaanasArora
Copy link
Contributor Author

MaanasArora commented Aug 27, 2025

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
Copy link
Member

seberg commented Aug 27, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
00 - Bug 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: incorrect promotion for Python integer inputs in ldexp
3 participants