SingleBlockManager.reindex almost unused, unusable

[jbrockmendel]

https://github.com/pandas-dev/pandas/blob/master/pandas/core/internals.py#L4447

SingleBlockManager.reindex has only code code branch hit in test coverage, immediately returns self. This is good because I think if it went further than that it would likely raise one of several errors.

The end of the method:

        if self._block.is_sparse:
            make_block = self._block.make_block_same_class

        block = make_block(new_values, copy=copy,
                           placement=slice(0, len(new_axis)))

        mgr = SingleBlockManager(block, new_axis)
        mgr._consolidate_inplace()
        return mgr

In the case where not self._block.is_sparse, then I'm pretty sure the author intended for make_block to point at the module-level make_block function. But instead it would raise an UnboundLocalError:

ser = pd.Series([1, 2, 3])
idx = ser.index * 2

>>> ser._data.reindex(idx)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pandas/core/internals.py", line 4518, in reindex
    block = make_block(new_values, copy=copy,
UnboundLocalError: local variable 'make_block' referenced before assignment

Moreover, the call to make_block passes a copy kwarg, which is not accepted by the module-level function.

So my hope is that it can be confirmed that this method is no longer needed and can removed.

[jreback]

well if you remove it and things don't break that would confirm it no?

[jbrockmendel]

well if you remove it and things don't break that would confirm it no?

Well it definitely wouldn't break any of the existing tests; is core.internals solidly enough tested that any un-covered code can be removed?

If you look at the test coverage the only path that gets hit is https://github.com/pandas-dev/pandas/blob/master/pandas/core/internals.py#L4464 that returns self almost immediately:

    def reindex(self, new_axis, indexer=None, method=None, fill_value=None,
                limit=None, copy=True):
        # if we are the same and don't copy, just return
        if self.index.equals(new_axis):
            if copy:
                return self.copy(deep=True)
            else:
                return self                                       # <-- only relevant path
        [... 21 more lines that are never hit ...]
        block = make_block(new_values, copy=copy,
                           placement=slice(0, len(new_axis)))

        mgr = SingleBlockManager(block, new_axis)
        mgr._consolidate_inplace()
        return mgr

[jbrockmendel]

It looks like the only places where BlockManager.reindex gets called are in Series.__init__ and SparseSeries.__init__

Series:

            elif isinstance(data, SingleBlockManager):
                if index is None:
                    index = data.index
                else:
                    data = data.reindex(index, copy=copy)

SparseSeries

            elif isinstance(data, SingleBlockManager):
                if dtype is not None:
                    data = data.astype(dtype)
                if index is None:
                    index = data.index.view()
                else:
                    data = data.reindex(index, copy=False)

In each case, an extra case after the if index is None clause elif data.index.equals(index): pass catches all remaining cases that currently pass into the else: block. Given that users shouldn't be passing SingleBlockManager manually, I think it'd be OK to require data.index.equals(index) in this case so we can remove BlockManager.reindex