BUG: Fix weak promotion with some mixed float/int dtypes #24681
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reorganizes/simplifies promotion a bit, but unfortunately this
means moving the promotion of our concrete DTypes with the abstract
"weak" ones into the concrete DTypes making the order of "who deals
with whom" fully defined by:
complex' > floats > pycomplex > pyfloat > ints > pyint > bool
where py<...> is the DType corresponding to the Python type.
There are two points here:
* "inexact" is the meaningful category not complex
* previously, the abstract DTypes `pyfloat`, etc. knew about the
concrete ones and there was some special handling for that.
(as mentioned above, this is gone now)
Why was it this complicated?! I think the initial code wanted to
deal with value-based promotion and the abstract DTypes could have
had a value attached to them.
There was thus more code later on, dealing with abstract DTypes
more concretely (rather than identical to the rest).
This is now gone, concrete DTypes promote basically the same as
their concrete counter-parts. This makes the promotion logic simpler
(and I think still just as correct), although the concrete type
promotion now needs to know about it.
|
A bit over my head, but I did check that this branch against SciPy |
|
@ev-br can you have a look at this one and see if we need to change anything in our implementation? |
|
Can we please review/merge this? It's a clear bug fix in something that is vital to move forward. (not that it is a big thing in that, but still) |
mattip
approved these changes
Sep 28, 2023
Co-authored-by: Matti Picus <matti.picus@gmail.com>
|
Thanks @seberg |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reorganizes/simplifies promotion a bit, but unfortunately this means moving the promotion of our concrete DTypes with the abstract "weak" ones into the concrete DTypes making the order of "who deals with whom" fully defined by:
where py<...> is the DType corresponding to the Python type. There are two points here:
pyfloat, etc. knew about the concrete ones and there was some special handling for that. (as mentioned above, this is gone now)Why was it this complicated?! I think the initial code wanted to deal with value-based promotion and the abstract DTypes could have had a value attached to them.
There was thus more code later on, dealing with abstract DTypes more concretely (rather than identical to the rest).
This is now gone, concrete DTypes promote basically the same as their concrete counter-parts. This makes the promotion logic simpler (and I think still just as correct), although the concrete type promotion now needs to know about it.