Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLN: Remove unused core.internals methods #19250

Merged
merged 14 commits into from
Jan 21, 2018
Merged
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 17 additions & 65 deletions pandas/core/internals.py
Original file line number Diff line number Diff line change
Expand Up @@ -303,10 +303,6 @@ def getitem_block(self, slicer, new_mgr_locs=None):
def shape(self):
return self.values.shape

@property
def itemsize(self):
return self.values.itemsize

@property
def dtype(self):
return self.values.dtype
Expand All @@ -327,21 +323,6 @@ def concat_same_type(self, to_concat, placement=None):
return self.make_block_same_class(
values, placement=placement or slice(0, len(values), 1))

def reindex_axis(self, indexer, method=None, axis=1, fill_value=None,
limit=None, mask_info=None):
"""
Reindex using pre-computed indexer information
"""
if axis < 1:
raise AssertionError(
'axis must be at least 1, got {axis}'.format(axis=axis))
if fill_value is None:
fill_value = self.fill_value

new_values = algos.take_nd(self.values, indexer, axis,
fill_value=fill_value, mask_info=mask_info)
return self.make_block(new_values, fastpath=True)

def iget(self, i):
return self.values[i]

Expand Down Expand Up @@ -937,11 +918,8 @@ 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'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

new = new.values

if hasattr(mask, 'reindex_axis'):
mask = mask.values
new = getattr(new, 'values', new)
mask = getattr(mask, 'values', mask)

# if we are passed a scalar None, convert it here
if not is_list_like(new) and isna(new) and not self.is_object:
Expand Down Expand Up @@ -1299,8 +1277,7 @@ def eval(self, func, other, errors='raise', try_cast=False, mgr=None):
orig_other = other
values = self.values

if hasattr(other, 'reindex_axis'):
other = other.values
other = getattr(other, 'values', other)

# make sure that we can broadcast
is_transposed = False
Expand Down Expand Up @@ -1448,11 +1425,8 @@ def where(self, other, cond, align=True, errors='raise',
if transpose:
values = values.T

if hasattr(other, 'reindex_axis'):
other = other.values

if hasattr(cond, 'reindex_axis'):
cond = cond.values
other = getattr(other, 'values', other)
cond = getattr(cond, 'values', cond)

# If the default broadcasting would go in the wrong direction, then
# explicitly reshape other instead
Expand Down Expand Up @@ -1929,7 +1903,6 @@ def should_store(self, value):


class DatetimeLikeBlockMixin(object):

@property
def _na_value(self):
return tslib.NaT
Expand Down Expand Up @@ -2603,6 +2576,10 @@ class DatetimeTZBlock(NonConsolidatableMixIn, DatetimeBlock):
_concatenator = staticmethod(_concat._concat_datetime)
is_datetimetz = True

@property
def tz(self):
return self.dtype.tz

def __init__(self, values, placement, ndim=2, **kwargs):

if not isinstance(values, self._holder):
Expand All @@ -2621,6 +2598,13 @@ def __init__(self, values, placement, ndim=2, **kwargs):
super(DatetimeTZBlock, self).__init__(values, placement=placement,
ndim=ndim, **kwargs)

def get_values(self, dtype=None):
"""
return object dtype as boxed values, such as Timestamps/Timedelta
"""
# Note: this overrides the NonConsolidatableMixIn implementation
return DatetimeBlock.get_values(self, dtype)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@TomAugspurger TomAugspurger Jan 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the idea was to

  1. Move DatetimeBlock.get_values here to the mixin
  2. 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def copy(self, deep=True, mgr=None):
""" copy constructor """
values = self.values
Expand All @@ -2634,14 +2618,6 @@ def external_values(self):
"""
return self.values.astype('datetime64[ns]').values

def get_values(self, dtype=None):
# return object dtype as Timestamps with the zones
if is_object_dtype(dtype):
f = lambda x: lib.Timestamp(x, tz=self.values.tz)
return lib.map_infer(
self.values.ravel(), f).reshape(self.values.shape)
return self.values

def _slice(self, slicer):
""" return a slice of my values """
if isinstance(slicer, tuple):
Expand Down Expand Up @@ -2767,10 +2743,6 @@ class SparseBlock(NonConsolidatableMixIn, Block):
def shape(self):
return (len(self.mgr_locs), self.sp_index.length)

@property
def itemsize(self):
return self.dtype.itemsize

@property
def fill_value(self):
# return np.nan
Expand Down Expand Up @@ -2895,22 +2867,6 @@ def shift(self, periods, axis=0, mgr=None):
return [self.make_block_same_class(new_values,
placement=self.mgr_locs)]

def reindex_axis(self, indexer, method=None, axis=1, fill_value=None,
limit=None, mask_info=None):
"""
Reindex using pre-computed indexer information
"""
if axis < 1:
raise AssertionError(
'axis must be at least 1, got {axis}'.format(axis=axis))

# taking on the 0th axis always here
if fill_value is None:
fill_value = self.fill_value
return self.make_block_same_class(self.values.take(indexer),
fill_value=fill_value,
placement=self.mgr_locs)

def sparse_reindex(self, new_index):
""" sparse reindex and return a new block
current reindex only works for float64 dtype! """
Expand Down Expand Up @@ -3311,7 +3267,7 @@ def apply(self, f, axes=None, filter=None, do_integrity_check=False,

aligned_args = dict((k, kwargs[k])
for k in align_keys
if hasattr(kwargs[k], 'reindex_axis'))
if hasattr(kwargs[k], 'values'))

for b in self.blocks:
if filter is not None:
Expand Down Expand Up @@ -4542,10 +4498,6 @@ def asobject(self):
"""
return self._block.get_values(dtype=object)

@property
def itemsize(self):
return self._block.values.itemsize

@property
def _can_hold_na(self):
return self._block._can_hold_na
Expand Down