ENH: Add isnat function and make comparison tests NAT specific #8421

seberg · 2016-12-26T22:01:46Z

Continuation of the discussion in gh-8413. First commit just adds a new isnat function, the second commit tries to clean up the warning filter in the comparison function.
Cleaning those up may be noticed by downstream projects (I am not sure). Backporting that part would be possible by implementing a local python implemented _isnat function instead (which is rather simple).

TODO:

I think the assert_equal function may say that a datetime NaT and a timedelta NaT are equal.

seberg · 2016-12-27T18:15:41Z

There is an old PR (gh-7856), which implements isfinite for all types including datetime, wondering whether we should do that instead, as well or not.

charris · 2017-01-02T23:10:23Z

numpy/testing/utils.py

+        if isnat(desired) or isnat(actual):
+            isdesnat = isnat(desired)
+            isactnat = isnat(actual)
+            if isdesnat or isactnat:


Could just do xor here: if isnat(desired) ^ isnat(actual):.

Or possibly more clearly, if isdesnat != isactnat:

charris · 2017-01-02T23:31:10Z

I think the assert_equal function may say that a datetime NaT and a timedelta NaT are equal.

Looks like that. I think we need to distinguish the two.

charris · 2017-01-02T23:38:14Z

Otherwise, LGTM.

charris · 2017-01-03T00:57:01Z

Note that this function could be implemented in pure python using views and integer comparisons. That might be an easier way to go. Same with the isfinite function, although the latter might be faster as a ufunc.

seberg · 2017-01-03T14:22:01Z

For isnat, agree we could make it a python function. The question I guess is whether having it a true ufunc is a good consistency gain. For isfininte, I would say its a no-go, since it already is a ufunc.

homu · 2017-01-10T10:08:16Z

☔ The latest upstream changes (presumably #8452) made this pull request unmergeable. Please resolve the merge conflicts.

charris · 2017-01-16T22:33:58Z

@seberg Needs rebase

seberg · 2017-01-21T16:55:46Z

Rebased, and fixed the datetime vs. timedelta stuff, still needs a few tests for the array case, maybe will add them in a bit.

charris · 2017-04-22T17:47:33Z

restart tests.

charris · 2017-05-05T18:32:53Z

@seberg The test for complex equality is failing. I'm going to kick this on to 1.14

charris · 2017-05-05T21:02:19Z

Oops, need this for the release to fix thread safety of comparison functions.

charris · 2017-05-05T21:03:40Z

@seberg Nag, nag. Consider yourself nagged ;)

seberg · 2017-05-06T08:45:25Z

Yeah... well don't try funny refactoring then. We could still also do the private python version of isnat though.

charris · 2017-05-06T17:48:47Z

numpy/core/tests/test_datetime.py

+        for t in np.typecodes["All"]:
+            if t in np.typecodes["Datetime"]:
+                continue
+


I'd drop the blank line, but not worth fixing here.

charris · 2017-05-06T17:50:05Z

numpy/testing/tests/test_utils.py

@@ -144,7 +145,10 @@ def test_recarrays(self):
        c['floupipi'] = a['floupi'].copy()
        c['floupa'] = a['floupa'].copy()

-        self._test_not_equal(c, b)
+        with suppress_warnings() as sup:


Unrelated fixup?

The safe comparison/suppression in the test filtered a bit more then just the NaT stuff (actually may have been there originally not for NaT, but for == operator warnings), so (not actually remembering it) removing it, the tests need to filter these warnings explicitly now. Not sure why this is the only one specifically though.

So this might cause some problems downstream? I'm not terribly concerned, the only way to find out is to try.

Hmm, looks like the FutureWarning will be gone in the 1.14 release.

It might cause problems, but maybe its also good, since maybe some project might run into the future warning and did not notice because of this....

charris · 2017-05-06T17:54:53Z

numpy/testing/utils.py

@@ -396,14 +397,24 @@ def assert_equal(actual,desired,err_msg='',verbose=True):
    except (TypeError, ValueError, NotImplementedError):
        pass

-    # Explicitly use __eq__ for comparison, ticket #2552
-    with suppress_warnings() as sup:


So this is the fix for thread safety?

charris · 2017-05-06T17:57:33Z

numpy/testing/utils.py

+    except (TypeError, ValueError, NotImplementedError):
+        pass
+
+    # Explicitly use __eq__ for comparison, ticket #2552


Dunno the comment was already there, probably ancient, maybe it was once np.equal or so.

charris · 2017-05-06T17:59:09Z

numpy/testing/utils.py

@@ -674,33 +685,12 @@ def assert_array_compare(comparison, x, y, err_msg='', verbose=True,
    x = array(x, copy=False, subok=True)
    y = array(y, copy=False, subok=True)

-    def safe_comparison(*args, **kwargs):


No longer needed? What replaces this?

Only needed for time comparisons then?

Well, it would filter a bit more (actuallylikely was first put here by Nathaniel or so for the == deprecations), but it seems the only test (after supporting NaT explicitely) that needed extra handling was this one test you comment on before. Not sure, maybe some of the deprecations are finished by now anyway?
Of course this might effect downstream a little here, which is why we did not go this way before after all.

Heh, I am wondering if it should be worrisome that only one test needed fixup.... but my brain refuses to really think about it right now ;)

charris · 2017-05-06T18:04:12Z

Looks good to me, just a few questions for clarification. Needs a 1.13 release note.

seberg · 2017-05-06T18:31:21Z

Hmmm, I just found another one of these in the masked array testutils. But they don't seem to support NaNs either, so not sure I shouldn't just move the warning suppression to the affected test. Or otherwise call the assert_equal from here, which figures NaNs and NaTs are equal.

charris · 2017-05-06T20:54:07Z

There is an old PR (gh-7856), which implements isfinite for all types including datetime,

Could probably implement that as a define to the relevant isnat after this.

charris · 2017-05-06T20:58:37Z

Still needs a release note.

These warning filters are not threadsafe, while it is nicer if the comparison functions can be called in a threaded environment since they do no explicite warning checking. Less filters are also cleaner in general, though may also mean that downstream projects see warnings that were previously hidden.

This may show warnings in downstream projects, which should likely not be there. We could also drop in the normal assert_equal there, which would support NaT, and NaN, but checks for scalarness, which this currently does not (which also could create problems). Works around the only test with this problem, by using the normal assert_equal there.

seberg · 2017-05-07T14:24:01Z

Added some notes. I removed the NaT filter from the masked array tests (and moved it to the tests), this might also affect downstream. We could also call into the normal assert equal function (which would be cleaner), however currently that checks for scalar, which could be more disruptive then the possible NaT warning. Or we do that, and add a flag to assert equal to skip the scalar check

EDIT: Anyway, there are few other options here (also adding a warning, that we will drop in assert_equal, which will change the scalar case). All of them should be pretty simple though.

charris · 2017-05-07T15:40:11Z

The masked testing utils are a bit of a mess anyway. In the future it would be nice if the testing functions were all clearly split between the usual numpy testing functions and those specific to masked arrays. I think what is done here is OK and we will deal with any problems that arise if/when they arise.

charris · 2017-05-07T15:41:19Z

Thanks Sebastian.

seberg · 2017-05-07T16:17:50Z

Thanks!

eric-wieser · 2017-11-27T05:58:20Z

numpy/testing/utils.py

+        # If both are NaT (and have the same dtype -- datetime or timedelta)
+        # they are considered equal.
+        if (isnat(desired) == isnat(actual) and
+                array(desired).dtype.type == array(actual).dtype.type):


This isn't right - it considers any two non-NAT times equal!

Fixed in #10096

charris added this to the 1.12.0 release milestone Dec 29, 2016

charris added 01 - Enhancement 00 - Bug component: numpy._core component: numpy.testing labels Dec 29, 2016

charris removed this from the 1.12.0 release milestone Dec 31, 2016

seberg added this to the 1.13.0 release milestone Jan 1, 2017

charris reviewed Jan 2, 2017

View reviewed changes

seberg force-pushed the isnat branch from e75b447 to 6200bce Compare January 21, 2017 16:54

seberg changed the title ~~WIP: Add isnat function and make comparison tests NAT specific~~ ENH: Add isnat function and make comparison tests NAT specific Jan 21, 2017

charris closed this Apr 22, 2017

charris reopened this Apr 22, 2017

charris modified the milestones: 1.14.0 release, 1.13.0 release May 5, 2017

charris modified the milestones: 1.13.0 release, 1.14.0 release May 5, 2017

charris mentioned this pull request May 5, 2017

Sometimes, supress_warnings misses one of its attributes #8413

Closed

seberg force-pushed the isnat branch from 6200bce to 40b1baa Compare May 6, 2017 08:45

charris reviewed May 6, 2017

View reviewed changes

seberg added 4 commits May 7, 2017 16:21

ENH: Add isnat function

eb8ead6

DOC: Document isnat addition and related warning changes

868aa75

seberg force-pushed the isnat branch from 40b1baa to 868aa75 Compare May 7, 2017 14:21

charris merged commit 39aaa2d into numpy:master May 7, 2017

seberg deleted the isnat branch May 7, 2017 16:07

eric-wieser mentioned this pull request Nov 27, 2017

BUG: numpy.testing.assert_equal does not test values of datetime64/timedelta64 scalars #10081

Closed

eric-wieser reviewed Nov 27, 2017

View reviewed changes

ENH: Add isnat function and make comparison tests NAT specific #8421

ENH: Add isnat function and make comparison tests NAT specific #8421

Conversation

seberg commented Dec 26, 2016 • edited Loading

seberg commented Dec 27, 2016

Choose a reason for hiding this comment

eric-wieser Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

charris commented Jan 2, 2017

charris commented Jan 2, 2017

charris commented Jan 3, 2017

seberg commented Jan 3, 2017

homu commented Jan 10, 2017

charris commented Jan 16, 2017

seberg commented Jan 21, 2017

charris commented Apr 22, 2017

charris commented May 5, 2017

charris commented May 5, 2017

charris commented May 5, 2017

seberg commented May 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris commented May 6, 2017

seberg commented May 6, 2017

charris commented May 6, 2017

charris commented May 6, 2017

seberg commented May 7, 2017 • edited Loading

charris commented May 7, 2017

charris commented May 7, 2017

seberg commented May 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg commented Dec 26, 2016 •

edited

Loading

eric-wieser Jan 4, 2017 •

edited

Loading

seberg commented May 7, 2017 •

edited

Loading