ENH,DEP: Add DTypePromotionError and finalize the == and != FutureWarning/Deprecation by seberg · Pull Request #22707 · numpy/numpy

seberg · 2022-12-01T16:55:53Z

Opening as a draft, because I think I should probably split it out the pieces (although it isn't as huge as one might think).

The PR effectively does a few things:

Add a new DTypePromotionError exception and use it for all promotion errors (DType or common instance where promotion shouldn't work).
Reorganize ufunc dispatching very mildly to chain this error when it occurs with a UFuncNoLoopError, also make the two-operand version inherit from the first.
Prohibit np.equal(timedelta, datetime), etc. to work. This is a very small break, because np.equal(timedelta, datetime, casting="unsafe") actually used to work.
Finalize the derprecation/futurewarning :). When dtypes are fundamentally not comparable, an array of all False or all True is returned.

There are some tiny things to (possibly consider):

Object arrays could cause a nested error. That could probably be mitigated, but since == calls == in the ufunc loop NumPy arrays can't fall into that trap easily.
The timedelta/datetime change is a "breaking change", I think its fine, it is just too absurd and ridiculously niche (with the need for casting=).

The main changes to think about:

Should we push this into the ufunc? This could be done. It would require either some hacks for void, or properly moving void comparisons into the ufunc code (which is a larger project probably). I think this could also be done at a later time.
What errors do we want? It might actually be sufficient to have a single reliable DTypeTypeError for everything. I wouldn't mind introducing one as a baseclass for both DTypePromotionError and the UFunc* errors (we can still delay exposing the UFunc* errors. We could probably also get away without DTypePromotionError , but I think I like it, it is pretty nicely and narrowly defined.

If promotion fails internally, this effectively means that there is no loop available for the given ufunc, so chain that error. We only do this for `InvalidPromotion` since we assume other errors may well be critical.

This finalize the deprecation warning, there are a couple of corner cases that we don't get as nicely as we could. E.g. we (for now?) manually create the all-false or all-true result array with a bit clunky subclass support (subclasses can always override `==` and `!=` though). We also keep some (existing) behavior for 0-D objects where we just return `NotImplemented`, which means the result should end up with Python booleans (which is probably just as well).

This allows correctly cathing the error, also adjust the the NoLoop error is used for the the "binary no loop error". For now, I think I may want to keep the casting error distinct.

mhvk

This looks good! It seems my main comment is about the name of the new exception...

mhvk · 2022-12-01T17:01:04Z

numpy/core/_internal.py

 import sys
 import warnings

+from ..exceptions import InvalidPromotion


Names are always hard and easy to argue about, but might PromotionError or DtypePromotionError be better?

I once heard the opinion that having "error" or "exception" inside an Exception is really redundant, but no strong opions.
I do agree that there is a point in including DType in the name though.

In principle agreed, but given that essentially all CPython exceptions have "Error" in the name... I think I also prefer having DType in the name. So, maybe DTypePromotionError?

mhvk · 2022-12-01T17:05:07Z

numpy/core/src/multiarray/arrayobject.c

+            Py_RETURN_NOTIMPLEMENTED;
+        }
+        /*
+         * Unfortunately, this path doesn't do the full __array_ufunc__


Does this matter? Didn't subclasses already get a chance when entering the ufunc?

Ah, I hadn't thought of that, but I think it still does because of nested ufunc calls. I.e. if Quantities would not implemented == but their __array_ufunc__ calls np.equal() on base-class arrays the error (correctly) is floated up, but the result (incorrectly) would not be wrapped.

EDIT: Should probably add a test that does implement __array_ufunc__ like that.

OK, I see, you mean something like np.array([MyClass(...), MyClass(...)], dtype=object)?

I was thinking of something like:

class MyArr(np.ndarray): def __array_ufunc__(self, ufunc, ops, ...): array_ops = cast_self_to_numpy(ops) np_result = ufunc(array_ops, ...) return MyArr.from_numpy(np_result) def __eq__(self, other): return super().__eq__(self, other) # gets called, but does nothing

where == does nothing and relies on __array_ufunc__. In that case ufunc(array_ops, ...) raises, so MyArr.from_numpy is never called (and thus the __array_ufunc__ call is effectively meaningless).

Ah, I see. The class would then also have to implement __array_wrap__ or write __eq__ better. I guess that just has to be the case, since np.equal and ndarray.__eq__ work differently.

I think the comment will just end up being confusing. How about

The following means that subclasses have to override __eq__ in order to override the output of comparisons of incompatible types; overriding __array_ufunc__ is itself not enough as that only helps to determine the output of np.equal, not what happens to errors raised by it (though those could be caught).

mhvk · 2022-12-01T17:08:39Z

numpy/core/src/multiarray/arrayobject.c

+         * so simply fill it with 0 or 1.
         */
-        result = _failed_comparison_workaround(self, other, cmp_op);
+        memset(PyArray_BYTES(res), cmp_op == Py_EQ ? 0 : 1, PyArray_NBYTES(res));


I'd tend to just use a standard fill function since this is not a critical path at all, but obviously not a big deal.

numpy/core/src/multiarray/arrayobject.c

mhvk · 2022-12-01T17:25:31Z

numpy/core/src/multiarray/datetime.c

+ * should promote (two datetimes should have a common datetime), but cannot.
+ * The promotion is thus valid, but fails.
+ * This is important, because `arr == arr` for these cannot reasonably return
+ * all ``False`` values.


Does it fail instead? Maybe extend sentence "... values, but should fail instead."?

Ah, should expand on this... The cast normally fails because it is considered "unsafe". The old hack is unnecessary because we now find the loop directly in either case.

mhvk · 2022-12-01T17:36:02Z

numpy/core/tests/test_multiarray.py

+    # Make sure they broadcast to test result shape, use random values, since
+    # the actual value should be ignored
+    arr1 = np.random.randint(5, size=100).astype(dt1)
+    arr2 = np.random.randint(5, size=99)[:, None].astype(dt2)


nitpick: np.newaxis is clearer.

numpy/core/tests/test_regression.py

mhvk · 2022-12-01T17:42:37Z

numpy/exceptions.py

+
+    Notes
+    -----
+    ``InvalidPromotion`` derives from ``TypeError`` and if it occurs within


replace "if it occurs within" with "in"

rephrasing a bit, the use of parentheses wasn't nice anyway.

mhvk · 2022-12-01T17:43:40Z

numpy/exceptions.py

+    Datetimes and complex numbers are incompatible classes and cannot be
+    promoted:
+
+    >>> np.result_type(np.dtype("M8", "D")


Example is scrambled; needs two np.dtype?

numpy/core/tests/test_multiarray.py

mhvk · 2022-12-01T18:02:04Z

p.s. This fails one astropy test, but it was one that tested the status quo; having this would be an improvement!

just silences a compiler warning.

Python does even crash when you get it wrong, so this is unnecessar

seberg · 2022-12-02T09:15:30Z

@mroeschke I am curious about pandas. I am seeing a lot of failures in this test:

Details

    @pytest.mark.filterwarnings("ignore:elementwise comp:DeprecationWarning")
    def test_get_indexer_non_unique_np_nats(self, np_nat_fixture, np_nat_fixture2):
        expected_missing = np.array([], dtype=np.intp)
        # matching-but-not-identical nats
        if is_matching_na(np_nat_fixture, np_nat_fixture2):
            # ensure nats are different objects
            index = Index(
                np.array(
                    ["2021-10-02", np_nat_fixture.copy(), np_nat_fixture2.copy()],
                    dtype=object,
                ),
                dtype=object,
            )
            # pass as index to prevent target from being casted to DatetimeIndex
            indexer, missing = index.get_indexer_non_unique(
                Index([np_nat_fixture], dtype=object)
            )
            expected_indexer = np.array([1, 2], dtype=np.intp)
            tm.assert_numpy_array_equal(indexer, expected_indexer)
            tm.assert_numpy_array_equal(missing, expected_missing)
        # dt64nat vs td64nat
        else:
            index = Index(
                np.array(
                    [
                        "2021-10-02",
                        np_nat_fixture,
                        np_nat_fixture2,
                        np_nat_fixture,
                        np_nat_fixture2,
                    ],
                    dtype=object,
                ),
                dtype=object,
            )
            # pass as index to prevent target from being casted to DatetimeIndex
>           indexer, missing = index.get_indexer_non_unique(
                Index([np_nat_fixture], dtype=object)
            )


>   ???
E   OverflowError: Integer overflow getting a common metadata divisor for NumPy datetime metadata [as] and [m].

Mostly failing due to incompatible time units (NumPy cannot convert between them and now raises an error somewhere deeper in index.get_indexer_non_unique()).

Failing for incompatible time units like that seems rather minor on first sight. We could make whatever comparison is failing here pass if we argue that the values cannot possibly be equal. (The DeprecationWarning didn't warn of that possibility, but if we are sure of the result it makes sense to change, IMO...)

Co-authored-by: Marten van Kerkwijk <mhvk@astro.utoronto.ca>

seberg · 2022-12-02T11:25:01Z

OK, decided to change to DTypePromotionError. I agree that DType should be in there and "InvalidPromotion" reads no or barely better than "PromotionError", so there is no reason not to follow the typical patter.

mhvk

Looks great. Only trivial comments.

mhvk · 2022-12-02T14:22:48Z

doc/release/upcoming_changes/22707.expired.rst

+------------------------------------
+The ``==`` and ``!=`` operators on arrays now always:
+
+* raise errors that occur during comparisons


Maybe good to give at least one example "such as arrays that have incompatible shapes"

mhvk · 2022-12-02T14:29:17Z

numpy/core/src/multiarray/arrayobject.c

+            Py_RETURN_NOTIMPLEMENTED;
+        }
+        /*
+         * Unfortunately, this path doesn't do the full __array_ufunc__


I think the comment will just end up being confusing. How about

The following means that subclasses have to override __eq__ in order to override the output of comparisons of incompatible types; overriding __array_ufunc__ is itself not enough as that only helps to determine the output of np.equal, not what happens to errors raised by it (though those could be caught).

numpy/core/src/multiarray/arrayobject.c

mhvk · 2022-12-02T15:02:07Z

numpy/core/src/multiarray/datetime.c

+     * The following errors do not currently use "DTypePromotionError".  This
+     * could be replaced (changing the OverflowError, though) with an explicit
+     * promotion error.
+     * This makes sense if we `arr_dt1 == arr_dt2` can reasonably return


"if we" -> "since"?

I have a bit of trouble following the comment. Maybe easier to write it as

It might make sense to replace the following errors with "DTypePromotionError", since we are here unable to convert one dtype to another. But one would have to think of whether one then wants `arr_dt1 == arr_dt2` to return an array of all `False`.

Aside: I don't think it should return an array of all False for either case, since it is just that
the conversion is difficult, not that it is not possible in principle. And some of the times
might well actually be equal (e.g., if they are 0?). So, I think an error is actually better.

Ah, good point about 0, I think that is a killer argument (until we wish to change the == implementation, but even then I think the argument makes sense).

numpy/core/tests/test_multiarray.py

mroeschke · 2022-12-02T22:16:40Z

@mroeschke I am curious about pandas. I am seeing a lot of failures in this test:

Mostly failing due to incompatible time units (NumPy cannot convert between them and now raises an error somewhere deeper in index.get_indexer_non_unique()).

From a glance given the exception, integer overflow due to time unit conversion, that exception reason generally matches what pandas will do for supporting other resolutions other than nanoseconds (e.g. datetime64[us]) so I want to say it's reasonable to this to fail.

mattip

A few suggestions and questions. Martin already did most of the review.

doc/release/upcoming_changes/22707.compatibility.rst

doc/release/upcoming_changes/22707.expired.rst

mattip · 2022-12-05T02:01:19Z

doc/release/upcoming_changes/22707.improvement.rst

+    np.result_type("M8[s]", np.complex128)
+
+Errors that occur while trying to find the correct promotion may raise a
+different error.


This sentence is incomplete. Either remove it, or give more detail: what other error class can be emitted?

Yeah, this is mainly the weirder datetime unit conversions that fail. But maybe that is all obscure enough that removing the sentence is best, since I don't want to give the example.
(Will remove it)

mattip · 2022-12-05T02:02:55Z

numpy/__init__.py


 from ._globals import _NoValue, _CopyMode
+from . import exceptions
+# Note that the following names are imported explicitly for backcompat:


Do we really have to export Python errors/warnings in the NumPy top-level namespace? If so those should be deprecated.

I agree we should follow-up with deprecating or at least hiding them from the main namespace. We could move the "namespace", I held off with that in the first PR, but the only thing that might notice is unpickling in an older version, and that is probably not worth the trouble for exceptions.

xref #22735

mattip · 2022-12-05T02:07:32Z

numpy/core/src/multiarray/arrayobject.c

+     * At this point we are locked into comparing (both are arrays) or
+     * something we would have tried to convert.


I don't understand the comment above: is the closing parenthesis in the wrong place? If we tried to convert it, did we fail? I would prefer it said this, but maybe it is not true.

Suggested change

* At this point we are locked into comparing (both are arrays) or

* something we would have tried to convert.

* At this point we comparing two ndarrays.

numpy/core/src/umath/dispatching.c

Co-authored-by: Matti Picus <matti.picus@gmail.com>

It is a bit too obscure, but explaining that it happens for datetime unit conversions seems more detailed then useful. Updated to include a last non-indent line, which the release notes machinery doesn't like otherwise...

mattip

LGTM. Just one typo. I will push a fix, feel free to merge once CI passes

doc/release/upcoming_changes/22707.expired.rst

mattip · 2022-12-05T20:01:58Z

numpy/__init__.py


 from ._globals import _NoValue, _CopyMode
+from . import exceptions
+# Note that the following names are imported explicitly for backcompat:


xref #22735

seberg · 2022-12-05T21:40:54Z

Wow, that was a quick turnaround on this, thanks Matti and Marten! Fingers crossed that downstream doesn't mind the FutureWarning's suddenly being fixed :).

If anyone has an opinion about the error naming or anything else, I am happy to follow up of course!

h-vetinari · 2022-12-06T08:31:50Z

I realise it's probably too late but given that this fixes a pretty long-standing issue (or at least annoyance), should this be backported to 1.24 (since it hasn't actually been released yet)?

charris · 2022-12-06T14:34:42Z

this fixes a pretty long-standing issue

That is an argument for not backporting :) If it only fixed one simple issue, maybe, but I think this is more expansive and should probably sit in the development branch for a while.

neutrinoceros · 2022-12-06T14:46:13Z

I'd like to add another argument for not backporting (as is, at least): this patch caused a regression downstream (see #22744)

mhvk · 2022-12-06T15:20:26Z

Indeed, it also fails one astropy test (see #22707 (comment)), though it is one that we are very happy to fix. But it means one definitely should not backport - much better if numpy version checks can be by minor number only, not micro.

h-vetinari · 2022-12-06T19:42:55Z

much better if numpy version checks can be by minor number only, not micro.

1.24.0 has not been released yet, so there'd be no need to drop to micro level.

I see the argument that it should sit in main for a while, though as pointed out the "breakage" often a positive thing due to removing the workarounds that were necessary for this issue.

I still find it a pity that we'll have to wait another ~6month for this. Had this PR been merged 2 weeks ago, its inclusion in 1.24.0 wouldn't be controversial.

But oh well, those 6 months will pass as well 🤷

mhvk · 2022-12-06T21:24:29Z

Hmm, I had not realized numpy 1.24 was not out. Still, probably best to give people time to fix bugs; astropy and others test against -dev just for that purpose, and a bit of extra time is good.

Xref numpy#22644 and numpy#22707 Update modules.pyi

seberg added 7 commits December 1, 2022 12:03

ENH: Add an InvalidPromotion exception

ea2f2d6

ENH: Ensure UFuncTypeError is raised when promotion fails

e21a205

If promotion fails internally, this effectively means that there is no loop available for the given ufunc, so chain that error. We only do this for `InvalidPromotion` since we assume other errors may well be critical.

TST: Adapt test for modification to == and != deprecation/futurewarning

84e4fff

MAINT: (equality) fixup subclass handling for new array-return path

ab1a6b9

TST: Add test to cover basic subclass support of new code paths

680a6f2

API: Always reject equal/not_equal for datetime/timedelta mix

84b428e

This allows correctly cathing the error, also adjust the the NoLoop error is used for the the "binary no loop error". For now, I think I may want to keep the casting error distinct.

seberg marked this pull request as draft December 1, 2022 16:56

seberg changed the title ~~ENH,DEP: Add InvalidPromotion error and *finally* finalize the == and != FutureWarning/Deprecation~~ ENH,DEP: Add InvalidPromotion error and finalize the == and != FutureWarning/Deprecation Dec 1, 2022

TST: Remove old deprecation test and convert/add new ones

4929777

seberg force-pushed the invalid-promotion-exc branch from 404ecb8 to 4929777 Compare December 1, 2022 16:58

mhvk reviewed Dec 1, 2022

View reviewed changes

seberg force-pushed the invalid-promotion-exc branch from 3952d1e to a6dc750 Compare December 1, 2022 18:18

MAINT: Fix pointer cast subclass check and for exceptions

0a638a9

just silences a compiler warning.

seberg force-pushed the invalid-promotion-exc branch from a6dc750 to 0a638a9 Compare December 1, 2022 19:59

MAINT: Remove pedantic check that exception is an exception

ba9136f

Python does even crash when you get it wrong, so this is unnecessar

seberg and others added 3 commits December 2, 2022 11:59

DOC,TST: address review comments and improve test coverage

fd30908

Co-authored-by: Marten van Kerkwijk <mhvk@astro.utoronto.ca>

MAINT: Rename to DTypePromotionError

708e1bf

DOC: Add release note for promotion/ and ==/!= expired deprecation

c8be159

seberg force-pushed the invalid-promotion-exc branch from 256d2ae to c8be159 Compare December 2, 2022 11:19

seberg changed the title ~~ENH,DEP: Add InvalidPromotion error and finalize the == and != FutureWarning/Deprecation~~ ENH,DEP: Add DTypePromotionError and finalize the == and != FutureWarning/Deprecation Dec 2, 2022

DOC: No need to repeat "error" after DTypePromotionError and line breaks

044c077

BUG: Add missing error check

7a00dbc

seberg marked this pull request as ready for review December 2, 2022 12:31

mhvk reviewed Dec 2, 2022

View reviewed changes

DOC: Tweak subclass in richcompare comments based on review

5d6c824

DOC: try to clarify datetime not using DTypePromotionError comment

75c7655

mattip reviewed Dec 5, 2022

View reviewed changes

seberg and others added 2 commits December 5, 2022 10:39

Apply suggestions from code review

d73e490

Co-authored-by: Matti Picus <matti.picus@gmail.com>

DOC: Remove last note from release note as per suggestion

37d95c8

It is a bit too obscure, but explaining that it happens for datetime unit conversions seems more detailed then useful. Updated to include a last non-indent line, which the release notes machinery doesn't like otherwise...

seberg force-pushed the invalid-promotion-exc branch from b601e8a to 37d95c8 Compare December 5, 2022 12:01

DOC: Tweak comparison code comment to be a bit more clear

d9b75bc

mattip approved these changes Dec 5, 2022

View reviewed changes

DOC: typo

7375ee6

seberg merged commit 38e7ab8 into numpy:main Dec 5, 2022

seberg deleted the invalid-promotion-exc branch December 5, 2022 21:41

This was referenced Dec 6, 2022

Deprecation warning when comparing a np.datetime64 scalar with a scalar #13548

Closed

array_equal should not raise a FutureWarning #9210

Closed

BUG: Comparing a datetime with a timedelta gives a deprecation warning #10095

Closed

neutrinoceros mentioned this pull request Dec 6, 2022

BUG: broken comparison between ndarray subclass (unyt) and strings #22744

Closed

mhvk mentioned this pull request Dec 6, 2022

Numpy dev 1.25 bugfixes astropy/astropy#14128

Merged

10 tasks

jbrockmendel mentioned this pull request Dec 7, 2022

BUG: SparseArray[dt64_weird_fill_value].astype(int64) pandas-dev/pandas#50087

Merged

5 tasks

jorisvandenbossche mentioned this pull request Dec 22, 2022

BUG/COMPAT: assert_* functions failing with nested arrays and latest numpy pandas-dev/pandas#50360

Open

BvB93 added a commit to BvB93/numpy that referenced this pull request May 8, 2023

TYP: Add the np.exceptions namespace and add DTypePromotionError

c6fdee0

Xref numpy#22644 and numpy#22707 Update modules.pyi

craig-warren mentioned this pull request Sep 18, 2023

Error with subgrids/cylinder_fs.py example and PyCharm gprMax/gprMax#367

Closed

cobaltt7 mentioned this pull request Oct 6, 2023

DOC: Remove outdated docs about NumPy's broadcasting pandas-dev/pandas#55427

Merged

ivirshup mentioned this pull request Dec 12, 2023

numpy regression in sparse indexing scverse/anndata#1254

Closed

3 tasks

pp-mo mentioned this pull request Dec 13, 2023

Avoid numpy deprecation for scalar array comparison SciTools/iris#5627

Closed

github-actions bot mentioned this pull request Sep 23, 2025

Link Checker Report SciTools/iris#6722

Closed

		* At this point we are locked into comparing (both are arrays) or
		* something we would have tried to convert.

	* At this point we are locked into comparing (both are arrays) or
	* something we would have tried to convert.
	* At this point we comparing two ndarrays.

Uh oh!

Conversation

seberg commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk left a comment

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!

seberg Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

Uh oh!

mhvk commented Dec 1, 2022

Uh oh!

seberg commented Dec 2, 2022

Uh oh!

seberg commented Dec 2, 2022

Uh oh!

mhvk left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mroeschke commented Dec 2, 2022

Uh oh!

mattip left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

seberg commented Dec 1, 2022 •

edited

Loading

seberg Dec 1, 2022 •

edited

Loading

h-vetinari commented Dec 6, 2022 •

edited

Loading