BUG: Fix that reduce-likes honor out always (and live in the future)#20793
Merged
charris merged 1 commit intonumpy:mainfrom Jan 12, 2022
Merged
BUG: Fix that reduce-likes honor out always (and live in the future)#20793charris merged 1 commit intonumpy:mainfrom
charris merged 1 commit intonumpy:mainfrom
Conversation
335bb0c to
fd1488e
Compare
Member
|
Does this need a backport? The issue is marked for 1.22.1 |
Member
Author
|
Yes, it needs a backport. Optimistically, we can all pull together in one direction on removing value-based promotion and pull a plug on all of this by the next release. |
mhvk
reviewed
Jan 11, 2022
Contributor
mhvk
left a comment
There was a problem hiding this comment.
It is a pretty evil hack, but also really nicely localized, so I think it is OK.
I guess to make it less hacky, one could take a view as a 1-dim, 1-element array (in which case it should not be moved, obviously). But it seems extra work for little benefit - in a way, it is good that this is clearly a hack,
Reducelikes should have lived in the future where the `out` dtype is correctly honoured always and used as one of the *inputs*. However, when legacy fallback occurs, this leads to problems because the legacy code path has 0-D fallbacks. There are two probable solutions to this: * Live with weird value-based stuff here even though it was never actually better especially for reducelikes. (enforce value-based promotion) * Avoid value based promotion completely. This does the second one, using a terrible hack by just mutating the dimension of `out` to tell the resolvers that value-based logic cannot be used. Is that hack safe? Yes, so long nobody has super-crazy custom type resolvers (the only one I know is pyerfa and they are fine, PyGEOS I think has no custom type resolver). It also relies on the GIL of course, but... The future? We need to ditch this value-based stuff, do annoying acrobatics with dynamically created DType classes, or something similar (so ditching seems best, it is topping my TODO list currently). Testing this is tricky, running the test: ``` python runtests.py -t numpy/core/tests/test_ufunc.py::TestUfunc::test_reducelike_out_promotes ``` triggers it, but because reducelikes do not enforce value-based promotion the failure can be "hidden" (which is why the test succeeds in a full test run). Closes numpygh-20739
fd1488e to
f435a33
Compare
Member
|
Well, let's give this a shot., Thanks Sebastian. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Reducelikes should have lived in the future where the
outdtypeis correctly honoured always and used as one of the inputs.
However, when legacy fallback occurs, this leads to problems because
the legacy code path has 0-D fallbacks.
There are two probable solutions to this:
actually better especially for reducelikes.
(enforce value-based promotion)
This does the second one, using a terrible hack by just mutating
the dimension of
outto tell the resolvers that value-based logiccannot be used.
Is that hack safe? Yes, so long nobody has super-crazy custom type
resolvers (the only one I know is PyGEOS and pyerfa and they are fine).
It also relies on the GIL of course, but...
The future? We need to ditch this value-based stuff, do annoying
acrobatics with dynamically created DType classes, or something similar
(so ditching seems best, it is topping my TODO list currently).
Testing this is tricky, running the test:
triggers it, but because reducelikes do not enforce value-based promotion
the failure can be "hidden" (which is why the test succeeds in a full test
run).
Closes gh-20739
@mhvk could you take a moment and see if this makes enough sense to you?