CLN: Remove unused core.internals methods #19250

jbrockmendel · 2018-01-15T18:48:20Z

De-duplicate some bits of DatetimeBlock and DatetimeTZBlock.

closes core.internals checking hasattr(item, 'reindex_axis') #19243
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

codecov · 2018-01-15T23:59:17Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ae96187). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #19250   +/-   ##
=========================================
  Coverage          ?   91.55%           
=========================================
  Files             ?      150           
  Lines             ?    48713           
  Branches          ?        0           
=========================================
  Hits              ?    44599           
  Misses            ?     4114           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`89.92% <100%> (?)`
#single	`41.74% <66.66%> (?)`

Impacted Files	Coverage Δ
pandas/core/internals.py	`94.95% <100%> (ø)`

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 ae96187...59b3e60. Read the comment docs.

jreback · 2018-01-16T00:08:32Z

pandas/core/internals.py

@@ -937,10 +918,10 @@ def putmask(self, mask, new, align=True, inplace=False, axis=0,

        new_values = self.values if inplace else self.values.copy()

-        if hasattr(new, 'reindex_axis'):


some of these may not be hit, I think I prefer the more idiomatic

new = getattr(new, 'values', new)

Sure. I'll check to see whether any value-less cases get through while I'm at it.

jreback · 2018-01-16T00:09:35Z

pandas/core/internals.py

@@ -1929,6 +1910,7 @@ def should_store(self, value):


 class DatetimeLikeBlockMixin(object):
+    freq = None  # compat with Datetimelike Index subclasses


put comments above this, but think this comment is out of place, meaning that unless you have internal knowledge of the codebase you won't have any idea what this is, rather put a 1-liner that explains

jreback · 2018-01-16T00:09:41Z

pandas/core/internals.py

@@ -2466,6 +2448,7 @@ class DatetimeBlock(DatetimeLikeBlockMixin, Block):
    __slots__ = ()
    is_datetime = True
    _can_hold_na = True
+    tz = None



jreback · 2018-01-16T00:10:22Z

pandas/core/internals.py

@@ -2603,6 +2586,12 @@ class DatetimeTZBlock(NonConsolidatableMixIn, DatetimeBlock):
    _concatenator = staticmethod(_concat._concat_datetime)
    is_datetimetz = True

+    get_values = DatetimeBlock.get_values  # override NonConsolidatableMixin


I'd rather you write an actual function here (that can call DatetimeBlock.get_values), but that has a doc-string and such

…used_internals

jorisvandenbossche · 2018-01-16T10:24:19Z

pandas/core/internals.py

@@ -1929,6 +1903,9 @@ def should_store(self, value):


 class DatetimeLikeBlockMixin(object):
+    # We add a dummy `freq` attribute here to make these block subclasses
+    # behave more like the DatetimelikeIndexOpsMixin implementations
+    freq = None


I don't understand the purpose for this. Why should blocks behave more like DatetimelineIndexOpsMixin ?

I see the point for adding tz (for a little bit of deduplication), but freq is never used? Why adding it?

I don't understand the purpose for this. Why should blocks behave more like DatetimelineIndexOpsMixin ?

@jorisvandenbossche Two parallel strands here.

a) #19147 and a bunch of others are moving towards having Series ops delegate to the Index subclass methods, which have careful implementations of timezone and type checking. But longer-term, it would be better for these methods to just delegate to e.g. self._data.blocks[0].__add__ (probably accessed more prettily) and have the implementations in the blocks. This will also make it easier for DataFrame ops to delegate correctly, see #18824. So ideally e.g. TimedeltaBlock and TimedeltaIndex would both inherit their arithmetic/comparison methods from TimedeltaArray or TimedeltaVector or something.

b) #19174 has PeriodArray subclass DatetimelikeOpsMixin, notes that it would be preferable to have the mixin class refactor out index-specific ops and separate the more array-like ops. I've been working along those lines, have found that small namespaces differences are the main thing standing in the way of this refactoring. So partly this is trying to smooth out those differences in a piece-meal way.

That definitely answers some question. Does it answer the right question?

I see the point for adding tz (for a little bit of deduplication), but freq is never used? Why adding it?

Purely for compat. This way DatetimeBlock._box_func == DatetimeTZBlock._box_func == DatetimeIndex._box_func.

Yes, I understand that we are thinking about that direction, but those things would be added in an array class, not the block. Anyhow, let's change this if we actually get there, not prematurely, as it is now only adding unused code (I mean the freq here).

Sounds good.

jreback · 2018-01-16T11:30:56Z

pandas/core/internals.py

@@ -2559,7 +2540,7 @@ def _try_coerce_result(self, result):

    @property
    def _box_func(self):
-        return tslib.Timestamp
+        return lambda x: tslib.Timestamp(x, freq=self.freq, tz=self.tz)


you can just return
```self.values._box_func()``, no need to add any attributes.

That would work for DatetimeTZBlock, but not for DatetimeBlock. This edit is to a) let them share the implementation and b) make the implementation match that of DatetimeIndex to move in the direction of sharing implementations.

…used_internals

jreback · 2018-01-18T00:24:42Z

pandas/core/internals.py

@@ -2467,6 +2440,10 @@ class DatetimeBlock(DatetimeLikeBlockMixin, Block):
    is_datetime = True
    _can_hold_na = True

+    # We add a dummy `tz` attribute here to make these block subclasses


let's remove this too (and the tz propert and changes to box_func).

OK. Not a fan of the idea of de-duplicating or just don't want to it in this PR?

jreback · 2018-01-18T11:13:48Z

pandas/core/internals.py

+        return object dtype as boxed values, such as Timestamps/Timedelta
+        """
+        # Note: this overrides the NonConsolidatableMixIn implementation
+        return DatetimeBlock.get_values(self, dtype)


so rather than do this, move the impl from DatetimeBlock here, you are calling a sub-class, not a good idea

You want to copy/paste the method? The original version in the PR was just get_values = DatetimeBlock.get_values to override the version from NonConsolidatableMixin (which is higher in the MRO).

IIUC, the idea was to

Move DatetimeBlock.get_values here to the mixin

Define DatetimeTZBlock.get_values as

def get_values(self, dtype=None): return DatetimeLikeBlockMixin.get_values(self, dtype)

Or, does changing DatetimeTZBlock's MRO to (DatetimeLikeBlockMixin, NonConsolidatableMixin, DatetimeBlock) break anything?

DatetimeLikeBlockMixin already defines get_values. The original version from this PR get_values = DatetimeBlock.get_values could equivalently be get_values = DatetimeLikeBlockMixin.get_values, which I guess is slightly better.

Or, does changing DatetimeTZBlock's MRO to (DatetimeLikeBlockMixin, NonConsolidatableMixin, DatetimeBlock) break anything?

Haven't tried that, but I expect it would break a lot b/c I think nothing from NonConsolidatableMixin would get used.

Ah that wouldn't work since you'd be inheriting from DatetimeLikeBlockMixin twice.

If this is just calling the super method what happens if you just remove it entirely?

If this is just calling the super method what happens if you just remove it entirely?

Argh. If this is removed entirely then the NonConsolidatableMixin.get_values implementation is used because it is at the top of the MRO. (I haven't checked how it breaks, but am assuming that whoever wrote the existing method overrode it for a reason). That's why all that is needed is a one-liner to point it towards the DatetimeBlock.get_values method.

jbrockmendel · 2018-01-18T21:58:19Z

Travis error looks like OSX; I'll sit tight until advised otherwise.

…used_internals

jreback · 2018-01-21T15:30:58Z

pandas/core/internals.py

@@ -2614,6 +2587,9 @@ def __init__(self, values, placement, ndim=2, dtype=None):
        super(DatetimeTZBlock, self).__init__(values, placement=placement,
                                              ndim=ndim)

+    # override NonConsolidatableMixIn implementation of get_values


remove this for now and revert get_values from the Block.

So just keep the unnecessary implementation currently in DatetimeTZBlock?

if you want to get this merged. yes. I really don't like this change the way it is. if you want to refactor into something more palable be my guest. but this PR would be on hold until then. so I would simply separate this (and to be honest it would have to be a convincing change).

Will do, am just curious because usually you like removing duplicate code.

yes, but in this case you are doing a completely orthognal change, which is already pretty spaghetti like, so a re-factor is in order. and separate that these changes.

…used_internals

jreback · 2018-01-21T18:00:20Z

my basic point is that the inheritance order needs to change for this, IOW NonConsolidated needs to be moved to the end of the MRO (and not the beginning). Then this will simply work (of course that might break other things)

jreback · 2018-01-21T18:01:17Z

ping on green.

jbrockmendel · 2018-01-21T18:33:47Z

That makes sense; thanks for explaining.

jreback · 2018-01-21T19:30:17Z

thanks!

jbrockmendel added 2 commits January 14, 2018 18:38

remove unused itemsize; de-duplicate DatetimeTZBlock methods

62bbf21

remove unused reindex_axis methods

495b522

jreback requested changes Jan 16, 2018

View reviewed changes

jreback added Internals Related to non-user accessible pandas implementation Clean labels Jan 16, 2018

jbrockmendel added 3 commits January 15, 2018 17:32

Merge branch 'master' of https://github.com/pandas-dev/pandas into un…

8d640a7

…used_internals

requested edits

b229159

extra comment

fb8c9ea

jorisvandenbossche reviewed Jan 16, 2018

View reviewed changes

jorisvandenbossche changed the title ~~Remove unused core.internals methods~~ CLN: Remove unused core.internals methods Jan 16, 2018

jreback requested changes Jan 16, 2018

View reviewed changes

jbrockmendel added 2 commits January 16, 2018 10:18

remove freq

6c4df2e

Merge branch 'master' of https://github.com/pandas-dev/pandas into un…

274e9d4

…used_internals

jreback requested changes Jan 18, 2018

View reviewed changes

revert box_func de-duplication per request

97a60dd

jreback requested changes Jan 18, 2018

View reviewed changes

revert duplicated get_values method

2256b5d

jbrockmendel added 2 commits January 19, 2018 09:21

Merge branch 'master' of https://github.com/pandas-dev/pandas into un…

d9d7829

…used_internals

Merge branch 'master' of https://github.com/pandas-dev/pandas into un…

43d92de

…used_internals

jreback requested changes Jan 21, 2018

View reviewed changes

jbrockmendel added 3 commits January 21, 2018 09:55

Merge branch 'master' of https://github.com/pandas-dev/pandas into un…

40bab35

…used_internals

revert to use redundant get_values method

d8f1104

revert whitespace change

59b3e60

jreback added this to the 0.23.0 milestone Jan 21, 2018

jreback approved these changes Jan 21, 2018

View reviewed changes

jreback merged commit 6ba65ca into pandas-dev:master Jan 21, 2018

jbrockmendel deleted the unused_internals branch January 23, 2018 04:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: Remove unused core.internals methods #19250

CLN: Remove unused core.internals methods #19250

jbrockmendel commented Jan 15, 2018 •

edited

Loading

codecov bot commented Jan 15, 2018 •

edited

Loading

jreback Jan 16, 2018

jbrockmendel Jan 16, 2018

jreback Jan 16, 2018

jreback Jan 16, 2018

jreback Jan 16, 2018

jorisvandenbossche Jan 16, 2018

jorisvandenbossche Jan 16, 2018

jbrockmendel Jan 16, 2018

jorisvandenbossche Jan 16, 2018

jbrockmendel Jan 16, 2018

jreback Jan 16, 2018

jbrockmendel Jan 16, 2018

jreback Jan 18, 2018

jbrockmendel Jan 18, 2018

jreback Jan 18, 2018

jbrockmendel Jan 18, 2018

TomAugspurger Jan 18, 2018 •

edited

Loading

jbrockmendel Jan 18, 2018

TomAugspurger Jan 18, 2018

jreback Jan 18, 2018

jbrockmendel Jan 19, 2018

jbrockmendel commented Jan 18, 2018

jreback Jan 21, 2018

jbrockmendel Jan 21, 2018

jreback Jan 21, 2018

jbrockmendel Jan 21, 2018

jreback Jan 21, 2018

jreback commented Jan 21, 2018

jreback commented Jan 21, 2018

jbrockmendel commented Jan 21, 2018

jreback commented Jan 21, 2018

		@@ -937,10 +918,10 @@ def putmask(self, mask, new, align=True, inplace=False, axis=0,

		new_values = self.values if inplace else self.values.copy()

		if hasattr(new, 'reindex_axis'):

		@@ -1929,6 +1910,7 @@ def should_store(self, value):


		class DatetimeLikeBlockMixin(object):
		freq = None # compat with Datetimelike Index subclasses

CLN: Remove unused core.internals methods #19250

CLN: Remove unused core.internals methods #19250

Conversation

jbrockmendel commented Jan 15, 2018 • edited Loading

codecov bot commented Jan 15, 2018 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger Jan 18, 2018 • edited Loading

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

jbrockmendel commented Jan 18, 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

jreback commented Jan 21, 2018

jreback commented Jan 21, 2018

jbrockmendel commented Jan 21, 2018

jreback commented Jan 21, 2018

jbrockmendel commented Jan 15, 2018 •

edited

Loading

codecov bot commented Jan 15, 2018 •

edited

Loading

TomAugspurger Jan 18, 2018 •

edited

Loading