core: try coerce result back to DatetimeBlock #22008

holymonson · 2018-07-21T09:13:26Z

closes dtype changed when DataFrame add DateOffset #21610 Inconsistent Timestamp arithmetic #8554 BUG: datetimelike subtract incorrect when broadcasting #12437
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Hang up until Block.eval reformed.

WillAyd · 2018-07-21T15:08:46Z

@jbrockmendel . Should this work with tz-aware date times as well?

jbrockmendel · 2018-07-21T15:24:34Z

Should this work with tz-aware date times as well?

Yes.

If there's any way at all to do this without poking into core.internals, that would be preferred. The Series version of this works correctly right? This should be doing something like operating column-wise to dispatch to the Series implementation (which itself dispatches to the DatetimeIndex implementation).

Can you include tests for subtraction while you're at it?

codecov · 2018-07-21T15:33:09Z

Codecov Report

Merging #22008 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #22008      +/-   ##
==========================================
+ Coverage   91.99%   91.99%   +<.01%     
==========================================
  Files         167      167              
  Lines       50578    50588      +10     
==========================================
+ Hits        46530    46540      +10     
  Misses       4048     4048

Flag	Coverage Δ
#multiple	`90.4% <100%> (ø)`	⬆️
#single	`42.17% <18.18%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/internals/__init__.py	`95.51% <100%> (+0.01%)`	⬆️

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 0828c25...21c1c7c. Read the comment docs.

jreback · 2018-07-21T15:37:17Z

pandas/core/internals.py

@@ -1362,10 +1368,15 @@ def eval(self, func, other, errors='raise', try_cast=False, mgr=None):
            values, values_mask, other, other_mask = self._try_coerce_args(
                transf(values), other)
        except TypeError:
+            # `Timestamp` operate with `DateOffset` must cast to `object`,


i would rather see you override eval in the appropriate blocks

holymonson · 2018-07-22T10:06:36Z

v2.0, override DatetimeBlock.eval.

Below test fails, but not a regression. Will look deeper later.

    def test_timestamp_df_sub_timestamp(self):
        expected = pd.DataFrame([pd.Timedelta('365d')])
        result = pd.DataFrame([pd.Timestamp('2018')]) - pd.Timestamp('2017')
        tm.assert_frame_equal(expected, result)

holymonson · 2018-07-22T10:22:29Z

docs said returning a new block, but indeed it returns a singleton list.

pandas/pandas/core/internals/__init__.py

Lines 1333 to 1336 in 37c7458

    
                   Returns 
        
                   ------- 
        
                   a new block, the result of the func 
        
                   """

pandas/pandas/core/internals/__init__.py

Lines 1455 to 1457 in 37c7458

    
           result = _block_shape(result, ndim=self.ndim) 
        
           return [self.make_block(result)]

holymonson · 2018-07-22T11:12:43Z

pandas/core/internals/__init__.py

+        block = super().eval(func, other, try_cast=try_cast, **kwargs)[0]
+        if try_cast:
+            if isinstance(other,
+                          (tslibs.Timestamp, np.datetime64, datetime, date)):


Is there an ABCTimestamp to check? At least 3 times use similar isinstance().

Is there an ABCTimestamp to check?

No, but Timestamp subclasses datetime, so could actually be removed from this check.

jbrockmendel · 2018-07-22T16:41:30Z

pandas/core/internals/__init__.py

@@ -2785,6 +2787,17 @@ def set(self, locs, values, check=False):

        self.values[locs] = values

+    def eval(self, func, other, try_cast=False, **kwargs):
+        block = super(DatetimeBlock, self).eval(func, other, try_cast=try_cast,


What happens here if instead of calling super eval you delegate to self.holder?

In DatetimeBlock - datetime case and DatetimeBlock + DateOffset case, it require the result block changing (DatetimeBlock -> TimeDeltaBlock , ObjectBlock -> DatetimeBlock or DatetimeTZBlock).
Delegating to self.holder couldn't change block type. Besides, self._holder is unaware of other.

In fact, what should be blamed is the last line in Block.eval carelessly make a wrong block. A proper fix should be

spliting a method to make block regarding to other.

override make_block with other as argument. But this will lead to self.make_block returning another type of block.

pandas/pandas/core/internals/__init__.py

Line 1456 in 0828c25

return [self.make_block(result)]

In fact, what should be blamed is the last line in Block.eval carelessly make a wrong block. A proper fix should be

There has been an effort recently to fix mismatches between the behaviors of arithmetic ops in Index vs Series vs DataFrame. For the timedelta and datetime cases, the canonical behavior is defined in TimedeltaIndex/DatetimeIndex.

The best case here to to avoid making this fix in core.internals but instead do it in core.DataFrame (or core.ops). The next base case is to have the FooBlock dispatch to the appropriate Index subclass to ensure the behavior is canonical.

That will be great, but it may need a big rewrite of eval and _try_coerce_args, cause now values is all coerced to ndarray[i8] for DatetimeLike. (May be benefited from numpy bi-ops, but confused to convert back.)

Is there an issue to xfer? This PR would depand on it.

holymonson · 2018-07-23T04:13:05Z

Hang up until Block.eval reformed. This will also close #8554 #12437.

jbrockmendel · 2018-07-23T05:13:06Z

pandas/tests/internals/test_internals.py

@@ -1238,7 +1238,6 @@ class TestCanHoldElement(object):
        (2**63, 'complex128'),
        (True, 'bool'),
        (np.timedelta64(20, 'ns'), '<m8[ns]'),
-        (np.datetime64(20, 'ns'), '<M8[ns]'),


Why removed? (Besides no is exempt sub working)?

This PR lead to datetime64 OP datetime64 = timedelta64, regardless of OP. Although meaningless, it will result datetime64 + datetime64 = timedelta64.

The assumption datetime64 OP datetime64 = datetime64 is meaningless for all OP. If allow this line, it will require extra OP check in DatetimeBlock.eval, which I don't think worth.

jbrockmendel · 2018-07-23T05:13:53Z

pandas/tests/internals/test_internals.py

+            (operator.mod, 'complex128'),
+            (operator.mod, '<m8[ns]'),
+            (operator.pow, 'bool'),
+        }


Pls avoid formatting changes where convenient, makes it harder for reviewers to identify the actual changes.

'<M8[ns]' - '<M8[ns]' = '<m8[ns]' , other binop are invalid.

jreback

I have a lot of concerns over the changes in the impl here.

jreback · 2018-07-28T13:48:33Z

pandas/core/internals/__init__.py

@@ -2737,7 +2739,7 @@ def _try_coerce_args(self, values, other):

    def _try_coerce_result(self, result):
        """ reverse of try_coerce_args """
-        if isinstance(result, np.ndarray):
+        if isinstance(result, (np.ndarray, Block)):


this is not a good change, please remove. the point is that these are scalars/arrays NEVER blocks.

jreback · 2018-07-28T13:48:59Z

pandas/core/internals/__init__.py

@@ -2920,6 +2933,8 @@ def _try_coerce_result(self, result):
        if isinstance(result, np.ndarray):
            if result.dtype.kind in ['i', 'f', 'O']:
                result = result.astype('M8[ns]')
+        elif isinstance(result, Block):


same, NEVER do this

jbrockmendel · 2018-08-17T18:43:26Z

Two of the bugs this addresses (#21610, #8554) were closed by #22163. The remaining issue #12437 is still open. @holymonson can this be simplified if only aimed at that issue?

holymonson · 2018-08-18T15:40:01Z

Two of the bugs this addresses (#21610, #8554) were closed by #22163. The remaining issue #12437 is still open. @holymonson can this be simplified if only aimed at that issue?

This Block-level fix is not good if dispatching to Series could already fix some of them. I would rather close this (Or you might want to take the testcases).

WillAyd added Datetime Datetime data dtype Timedelta Timedelta data type labels Jul 21, 2018

jreback requested changes Jul 21, 2018

View reviewed changes

holymonson force-pushed the timestamp_df-add-offset branch from ad96a28 to a75f375 Compare July 22, 2018 09:44

holymonson changed the title ~~core: coerce result to dtype in Block.eval if dtype explicitly given~~ core: try coerce result back to DatetimeBlock Jul 22, 2018

holymonson commented Jul 22, 2018

View reviewed changes

holymonson force-pushed the timestamp_df-add-offset branch from 4c36407 to 8a3a20e Compare July 22, 2018 15:00

jbrockmendel reviewed Jul 22, 2018

View reviewed changes

jbrockmendel reviewed Jul 23, 2018

View reviewed changes

holymonson added 4 commits July 23, 2018 13:51

core: try coerce result back to DatetimeBlock

a52db10

core: cast result to TimeDeltaBlock if needed

36794ed

internals: remove invalid test

98c3ac9

'<M8[ns]' - '<M8[ns]' = '<m8[ns]' , other binop are invalid.

revert style change

21c1c7c

holymonson force-pushed the timestamp_df-add-offset branch from c82ea08 to 21c1c7c Compare July 23, 2018 05:55

jreback requested changes Jul 28, 2018

View reviewed changes

holymonson closed this Aug 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: try coerce result back to DatetimeBlock #22008

core: try coerce result back to DatetimeBlock #22008

holymonson commented Jul 21, 2018 •

edited

Loading

WillAyd commented Jul 21, 2018

jbrockmendel commented Jul 21, 2018

codecov bot commented Jul 21, 2018 •

edited

Loading

jreback Jul 21, 2018

holymonson commented Jul 22, 2018

holymonson commented Jul 22, 2018

holymonson Jul 22, 2018

jbrockmendel Jul 22, 2018

jbrockmendel Jul 22, 2018

holymonson Jul 22, 2018

jbrockmendel Jul 22, 2018

holymonson Jul 23, 2018

holymonson commented Jul 23, 2018

jbrockmendel Jul 23, 2018

holymonson Jul 23, 2018

jbrockmendel Jul 23, 2018

jreback left a comment

jreback Jul 28, 2018

jreback Jul 28, 2018

jbrockmendel commented Aug 17, 2018

holymonson commented Aug 18, 2018

core: try coerce result back to DatetimeBlock #22008

core: try coerce result back to DatetimeBlock #22008

Conversation

holymonson commented Jul 21, 2018 • edited Loading

WillAyd commented Jul 21, 2018

jbrockmendel commented Jul 21, 2018

codecov bot commented Jul 21, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

holymonson commented Jul 22, 2018

holymonson commented Jul 22, 2018

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

holymonson commented Jul 23, 2018

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

jbrockmendel commented Aug 17, 2018

holymonson commented Aug 18, 2018

holymonson commented Jul 21, 2018 •

edited

Loading

codecov bot commented Jul 21, 2018 •

edited

Loading