BUG GH23282 calling min on series of NaT returns NaT #23289

JustinZhengBC · 2018-10-23T03:38:54Z

closes .min() on a series of NaTs returns nan, while .max() returns NaT #23282
tests passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

For max, NaT values are filled with the lowest possible value. For min, they are filled with the highest possible value. The problem is that only the lowest possible value is recognized as NaT. Since nanops.py is responsible for assigning the highest value to NaT when min is called, it should also be responsible for translating it to NaT when appropriate.

pep8speaks · 2018-10-23T03:38:57Z

Hello @JustinZhengBC! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/core/nanops.py !
There are no PEP8 issues in the file pandas/tests/series/test_datetime_values.py !

Comment last updated on October 25, 2018 at 23:07 Hours UTC

WillAyd

Please be sure to always add tests first and foremost

sinhrks · 2018-10-23T04:46:45Z

pandas/core/nanops.py

@@ -718,6 +718,8 @@ def reduction(values, axis=None, skipna=True, mask=None):
                result = np.nan
        else:
            result = getattr(values, meth)(axis)
+            if is_integer(result) and result == _int64_max:


It looks affect to integer dtype, pd.Series([_int64_max]).min() / max()?

Fixed, now it only applies the conversion from _int64_max to NaT if given an appropriate dtype.

sinhrks · 2018-10-23T04:47:48Z

pandas/tests/series/test_datetime_values.py

@@ -509,3 +509,8 @@ def test_dt_timetz_accessor(self, tz_naive_fixture):
                           time(22, 14, tzinfo=tz)])
        result = s.dt.timetz
        tm.assert_series_equal(result, expected)
+
+    def test_minmax_nat(self):


can u add test for timedelta dtype and DataFrame (#10390)

Added more tests, but this PR does not fix #10390

jreback · 2018-10-23T09:14:52Z

pandas/core/nanops.py

@@ -718,6 +718,9 @@ def reduction(values, axis=None, skipna=True, mask=None):
                result = np.nan
        else:
            result = getattr(values, meth)(axis)
+            if (is_integer(result) and is_datetime_or_timedelta_dtype(dtype)


this needs handling not here but in _wrap_resulf where a scalar should be turned into NaT if it’s null and of the correct dtype

pandas/tests/series/test_datetime_values.py

codecov · 2018-10-23T12:43:21Z

Codecov Report

Merging #23289 into master will decrease coverage by <.01%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master   #23289      +/-   ##
==========================================
- Coverage   92.22%   92.22%   -0.01%     
==========================================
  Files         169      169              
  Lines       51258    51266       +8     
==========================================
+ Hits        47274    47281       +7     
- Misses       3984     3985       +1

Flag	Coverage Δ
#multiple	`90.66% <93.75%> (-0.01%)`	⬇️
#single	`42.23% <43.75%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/nanops.py	`95.05% <93.75%> (-0.15%)`	⬇️
pandas/core/series.py	`93.91% <0%> (-0.01%)`	⬇️
pandas/core/arrays/sparse.py	`91.84% <0%> (ø)`	⬆️
pandas/core/arrays/datetimes.py	`97.46% <0%> (ø)`	⬆️
pandas/core/dtypes/cast.py	`89.28% <0%> (+0.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 437f31c...a70b903. Read the comment docs.

jreback · 2018-10-24T12:05:22Z

i pushed a commit. have a look.

TomAugspurger

Probably needs a release note.

TomAugspurger · 2018-10-25T20:22:49Z

pandas/core/nanops.py

@@ -346,7 +350,7 @@ def nanany(values, axis=None, skipna=True, mask=None):
    >>> nanops.nanany(s)
    False
    """
-    values, mask, dtype, _ = _get_values(values, skipna, False, copy=skipna,
+    values, mask, dtype, _, _ = _get_values(values, skipna, False, copy=skipna,
                                         mask=mask)


listing error I would guess.

TomAugspurger · 2018-10-25T20:25:22Z

pandas/core/nanops.py

    """ wrap our results if needed """

    if is_datetime64_dtype(dtype):
        if not isinstance(result, np.ndarray):
+            if result == fill_value:


Is it assumed that fill_value is not NA? If not, this will be wrong, since fill_value will never equal fill_value.

Should we assert that it's not NA?

so this can't be for an i8 type by definition. but yes we can assert it.

jreback

@JustinZhengBC can you add a whatsnew note & some asserts. ping on green.

jreback · 2018-10-26T00:37:49Z

pandas/core/nanops.py

    """ wrap our results if needed """

    if is_datetime64_dtype(dtype):
        if not isinstance(result, np.ndarray):
+            if result == fill_value:


so this can't be for an i8 type by definition. but yes we can assert it.

TomAugspurger · 2018-10-26T20:58:32Z

doc/source/whatsnew/v0.24.0.txt

@@ -1020,6 +1020,7 @@ Datetimelike
 - Bug in :func:`to_datetime` with an :class:`Index` argument that would drop the ``name`` from the result (:issue:`21697`)
 - Bug in :class:`PeriodIndex` where adding or subtracting a :class:`timedelta` or :class:`Tick` object produced incorrect results (:issue:`22988`)
 - Bug in :func:`date_range` when decrementing a start date to a past end date by a negative frequency (:issue:`23270`)
+- Bug in :func:`min` which would return ``NaN`` instead of ``NaT`` when called on a series of ``NaT`` (:issue:`23282`)


Was the bug in the builtin min from the standard library, or Series.min? Right now, you're linking to the builtin.

It was to Series.min. I think it's fixed now

JustinZhengBC · 2018-10-27T06:25:59Z

@jreback I added a whatsnew note and an assert in the datetime64 case (adding the assert to the timedelta64 case causes tests to fail)

jreback · 2018-10-28T02:58:01Z

lgtm. @WillAyd over to you.

WillAyd · 2018-10-28T22:31:19Z

Thanks @JustinZhengBC !

…y_tests * repo_org/master: (52 commits) ENH: Allow rename_axis to specify index and columns arguments (pandas-dev#20046) STY: proposed isort settings [ci skip] [skip ci] [ciskip] [skipci] (pandas-dev#23366) MAINT: Remove extraneous test.parquet file CLN: Follow-up comments to pandas-devgh-23392 (pandas-dev#23401) BUG GH23282 calling min on series of NaT returns NaT (pandas-dev#23289) unpin openpyxl (pandas-dev#23361) REF: collect ops dispatch functions in one place, try to de-duplicate SparseDataFrame methods (pandas-dev#23060) CLN: Remove pandas.tools module (pandas-dev#23376) CLN: Remove some dtype methods from API (pandas-dev#23390) CLN: Cleanup toplevel namespace shims (pandas-dev#23386) DOC: fixup whatsnew note for GH21394 (pandas-dev#23355) Fix import format at pandas/tests/extension directory (pandas-dev#23365) DOC: Remove Series.sortlevel from api.rst (pandas-dev#23395) API: Disallow dtypes w/o frequency when casting (pandas-dev#23392) BUG/TST/REF: Datetimelike Arithmetic Methods (pandas-dev#23215) STYLE: lint add np.nan* funcs to cython_table (pandas-dev#22109) Run Isort on tests/util single PR (pandas-dev#23347) BUG: Fix date_range overflow (pandas-dev#23345) Run Isort on tests/arrays single PR (pandas-dev#23346) ...

JustinZhengBC force-pushed the BUG-23282 branch from ab9313b to 44eb8d8 Compare October 23, 2018 03:39

WillAyd requested changes Oct 23, 2018

View reviewed changes

WillAyd added the Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate label Oct 23, 2018

sinhrks reviewed Oct 23, 2018

View reviewed changes

jreback requested changes Oct 23, 2018

View reviewed changes

JustinZhengBC added 3 commits October 23, 2018 11:36

BUG-23282 calling min on series of NaT returns NaT

dcce77c

BUG-23282 Add tests

ef91c24

BUG-23282 Add additional tests

8caeaed

JustinZhengBC force-pushed the BUG-23282 branch 2 times, most recently from 3f35609 to 95f3bf6 Compare October 23, 2018 18:40

BUG-23282 Move check to _wrap_result and add bug number to tests

ce0a7ee

JustinZhengBC force-pushed the BUG-23282 branch from 95f3bf6 to ce0a7ee Compare October 23, 2018 19:46

jreback added 2 commits October 24, 2018 08:03

Merge branch 'master' into PR_TOOL_MERGE_PR_23289

123ff25

use fill_value directly

a1bde9c

jreback added this to the 0.24.0 milestone Oct 24, 2018

jreback added the Bug label Oct 24, 2018

BUG-23282 add more underscores

9d6dca3

TomAugspurger reviewed Oct 25, 2018

View reviewed changes

BUG-23282 Fix linting

314b2bc

jreback requested changes Oct 26, 2018

View reviewed changes

JustinZhengBC added 2 commits October 25, 2018 20:43

BUG-23282 Add whatsnew and asserts

15e5e87

Merge branch 'master' into BUG-23282

e45d908

TomAugspurger reviewed Oct 26, 2018

View reviewed changes

JustinZhengBC added 2 commits October 26, 2018 14:58

BUG-23282 Fix whatsnew and assert

815da44

Fix merge conflict

a70b903

jreback approved these changes Oct 28, 2018

View reviewed changes

WillAyd approved these changes Oct 28, 2018

View reviewed changes

WillAyd merged commit 360e727 into pandas-dev:master Oct 28, 2018

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018

BUG GH23282 calling min on series of NaT returns NaT (pandas-dev#23289)

cab464a

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG GH23282 calling min on series of NaT returns NaT (pandas-dev#23289)

e3d07a1

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG GH23282 calling min on series of NaT returns NaT (pandas-dev#23289)

c5c18b3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG GH23282 calling min on series of NaT returns NaT #23289

BUG GH23282 calling min on series of NaT returns NaT #23289

JustinZhengBC commented Oct 23, 2018 •

edited by WillAyd

Loading

pep8speaks commented Oct 23, 2018 •

edited

Loading

WillAyd left a comment

sinhrks Oct 23, 2018

JustinZhengBC Oct 23, 2018

sinhrks Oct 23, 2018

JustinZhengBC Oct 23, 2018

jreback Oct 23, 2018

codecov bot commented Oct 23, 2018 •

edited

Loading

jreback commented Oct 24, 2018

TomAugspurger left a comment

TomAugspurger Oct 25, 2018

TomAugspurger Oct 25, 2018

jreback Oct 26, 2018

jreback left a comment

jreback Oct 26, 2018

TomAugspurger Oct 26, 2018

JustinZhengBC Oct 27, 2018

JustinZhengBC commented Oct 27, 2018

jreback commented Oct 28, 2018

WillAyd commented Oct 28, 2018

BUG GH23282 calling min on series of NaT returns NaT #23289

BUG GH23282 calling min on series of NaT returns NaT #23289

Conversation

JustinZhengBC commented Oct 23, 2018 • edited by WillAyd Loading

pep8speaks commented Oct 23, 2018 • edited Loading

Comment last updated on October 25, 2018 at 23:07 Hours UTC

WillAyd left a 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

codecov bot commented Oct 23, 2018 • edited Loading

Codecov Report

jreback commented Oct 24, 2018

TomAugspurger left a 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

jreback left a 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

JustinZhengBC commented Oct 27, 2018

jreback commented Oct 28, 2018

WillAyd commented Oct 28, 2018

JustinZhengBC commented Oct 23, 2018 •

edited by WillAyd

Loading

pep8speaks commented Oct 23, 2018 •

edited

Loading

codecov bot commented Oct 23, 2018 •

edited

Loading