MAINT: Implement new style promotion for np.result_type, etc.
#18676
Conversation
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
f3979d0 to
e55b000
Compare
numpy/core/src/multiarray/methods.c
Outdated
| if ((PyArray_NDIM(self) != PyArray_NDIM(ret)) && | ||
| DEPRECATE_FUTUREWARNING( | ||
| "casting an array to a subarray dtype " | ||
| "will not using broadcasting in the future, but cast each " |
| /* | ||
| * The following functions define the "common DType" for the abstract dtypes. | ||
| * | ||
| * Note that the logic with resupt to the "higher" dtypes such as floats |
| * python float, int, or complex. | ||
| * An array using this flag must be a temporary array that can never | ||
| * leave the C internals of NumPy. Even if it does, ENSUREARRAY is | ||
| * absolutely safe to abuse, since it alraedy is be a base class array :). |
There was a problem hiding this comment.
alraedy -> already?
is be a base class -> is a base class?
e55b000 to
40f2cc4
Compare
|
Thanks Tyler, let me know if you have any comments. I should round this off (mainly add tests, and scan through this). Although, I will read through the NEP 43 draft first now, since promotion in ufuncs is also confusing me again and that document also needs to settle soon. |
np.result_type, etc.np.result_type, etc.
|
Ping @mattip if you have some time please have a look at this. |
mattip
reviewed
Apr 22, 2021
| @@ -15,6 +15,8 @@ typedef struct coercion_cache_obj { | |||
| int depth; /* the dimension at which this object was found. */ | |||
| } coercion_cache_obj; | |||
|
|
|||
| NPY_NO_EXPORT PyArray_DTypeMeta * | |||
| npy_discover_dtype_from_pytype(PyTypeObject *pytype); | |||
There was a problem hiding this comment.
Where is this used outside of array_coercion.c?
There was a problem hiding this comment.
Thanks, must have been some transitionary thing.
There was a problem hiding this comment.
Can we delete these lines if they are not needed?
This implements a new style promotion scheme for mutiple inputs as are passed to `np.result_type`. It should be noted that result-type heavily relies on "value-based casting" logic/promotion logic. This fact is inherited to many places. When user DTypes are introduced some of these places should potentially use "weak DType" logic instead if the input is a Python scalar. This is *not* yet implemented. It would be necessary to mark these the same way as is currently done for `np.result_type`. This type of "marking" will generally be necessary in a few places, even if may get away with just switching the logic everywhere.
This doesn't serve any purpose anymore. The new code is now always used. (In very few cases this may lead to small slowdowns, this should only matter in ufuncs where it doesn't seem to matter enough to worry about it.)
9ec2f60 to
c5a08e7
Compare
Base the "magic" value of the double, and use nextafter even for the equal condition to avoid rounding during the cast to longdouble. This might further be necessary because the code uses double constants and not longdouble ones (not sure about this). Using nextafter on the double value should put us firmly on the right side and nobody should care about the few ulps in between those values.
c5a08e7 to
1d3b9b2
Compare
Co-authored-by: Matti Picus <matti.picus@gmail.com>
|
Sorry for missing to update this more earlier. I added some new tests parametrizations, hopefully that will clear out the missed coverage. |
mattip
reviewed
May 16, 2021
mattip
reviewed
May 16, 2021
mattip
reviewed
May 16, 2021
mattip
reviewed
May 16, 2021
(also delete those lines I forgot to delete)
81ea7ba to
2b7b256
Compare
|
Thanks Sebastian. |
1 task
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 implements a new style promotion scheme for mutiple inputs
as are passed to
np.result_type. It should be noted thatresult-type heavily relies on "value-based casting" logic/promotion
logic.
This fact is inherited to many places. When user DTypes are introduced
some of these places should potentially use "weak DType" logic instead
if the input is a Python scalar.
This is not yet implemented. It would be necessary to mark these
the same way as is currently done for
np.result_type.This type of "marking" will generally be necessary in a few places,
even if may get away with just switching the logic everywhere.
Marking as draft, since I am planning to probably write some small tests (there are some exceedingly rare changes in behaviour). Also, probably moving some of the new code to the new
common_dtype.cfile. But I don't expect any larger changes right now, so putting it out there.I expect the doc-tests will fail, this is OK, the doctests need to be adapted. There is a very mild slowdown in ufunc loop lookup due to this. That seems OK, it probably is around the same or less than the general speedup in gh-15271. Eventually it would largely go away again in the ufunc refactor (although the exact speed impact of that is unknown, we are talking about overheads of at most 10-20% of the cheapest possible ufunc calls...).