Skip to content

Remove some dead code in core/arrays/interval.py #46128

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.

Sign up for GitHub

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

Closed
wants to merge 9 commits into from
Closed

Remove some dead code in core/arrays/interval.py #46128

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9e57d6d
fix column_arrays for array manager
Dr-Irv Dec 21, 2021
2abbc57
merge with upstream/main
Dr-Irv Jan 15, 2022
99158b1
Merge remote-tracking branch 'upstream/main'
Dr-Irv Jan 24, 2022
0f4130d
Merge remote-tracking branch 'upstream/main'
Dr-Irv Feb 4, 2022
24ecfe2
Merge remote-tracking branch 'upstream/main'
Dr-Irv Feb 8, 2022
64f00d5
Merge remote-tracking branch 'upstream/main'
Dr-Irv Feb 9, 2022
1ee6e00
Merge remote-tracking branch 'upstream/main'
Dr-Irv Feb 19, 2022
5761bf2
Merge remote-tracking branch 'upstream/main'
Dr-Irv Feb 23, 2022
84b119f
remove dead code in arrays/interval.py
Dr-Irv Feb 23, 2022
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
8 changes: 2 additions & 6 deletions pandas/core/arrays/interval.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1664,12 +1664,8 @@ def _from_combined(self, combined: np.ndarray) -> IntervalArray:
nc = combined.view("i8").reshape(-1, 2)

dtype = self._left.dtype
if needs_i8_conversion(dtype):
new_left = type(self._left)._from_sequence(nc[:, 0], dtype=dtype)
new_right = type(self._right)._from_sequence(nc[:, 1], dtype=dtype)
else:
new_left = nc[:, 0].view(dtype)
new_right = nc[:, 1].view(dtype)
new_left = nc[:, 0].view(dtype)
new_right = nc[:, 1].view(dtype)
Copy link
Member

Choose a reason for hiding this comment

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

i think this will break if self._left is a DTA/TDA?

Copy link
Member

Choose a reason for hiding this comment

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

I'm honestly not sure what type _left and _right has (in #44922 I assumed np.ndarray but added a note that this is probably not correct - needs some mypy ignore comments). In your attempt at interval.pyi, you marked it as on of np.ndarray, "DatetimeArray", "TimedeltaArray".
https://github.com/pandas-dev/pandas/pull/41059/files#diff-fb052ef16d456d32858ec68d7e9449d4749166ea97c9a79b22d0ccf233dbb3a4R205

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think this will break if self._left is a DTA/TDA?

Well, that's why I did the test. I don't think that self._left can be a DTA or TDA. I guess there is other logic that causes that never to be the case, and the tests seem to confirm that.

Copy link
Member

Choose a reason for hiding this comment

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

dti = pd.date_range("2016-01-01", periods=3)
ii = pd.IntervalIndex.from_breaks(dti)
ser = pd.Series(ii)

ser.unique()  # <- gets here with DTA for self._left

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the dead code doesn't go there. The old code is:

        if needs_i8_conversion(dtype):
            new_left = type(self._left)._from_sequence(nc[:, 0], dtype=dtype)
            new_right = type(self._right)._from_sequence(nc[:, 1], dtype=dtype)
        else:
            new_left = nc[:, 0].view(dtype)
            new_right = nc[:, 1].view(dtype)

The new code is just:

        new_left = nc[:, 0].view(dtype)
        new_right = nc[:, 1].view(dtype)

In your example, the test needs_i8_conversion(dtype) is False

Here is what happens with 1.4.0 and this PR:

>>> import pandas as pd
>>> pd.__version__
'1.4.0'
>>> dti = pd.date_range("2016-01-01", periods=3)
>>> ii = pd.IntervalIndex.from_breaks(dti)
>>> ser = pd.Series(ii)
>>> ser.unique()
<IntervalArray>
[(2016-01-01, 2016-01-02], (2016-01-02, 2016-01-03]]
Length: 2, dtype: interval[datetime64[ns], right]

With the PR:

>>> import pandas as pd
>>> pd.__version__
'1.5.0.dev0+401.g84b119faea'
>>> dti = pd.date_range("2016-01-01", periods=3)
>>> ii = pd.IntervalIndex.from_breaks(dti)
>>> ser = pd.Series(ii)
>>> ser.unique()
<IntervalArray>
[(2016-01-01, 2016-01-02], (2016-01-02, 2016-01-03]]
Length: 2, dtype: interval[datetime64[ns], right]

That's why we can remove the if test.

Copy link
Member

Choose a reason for hiding this comment

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

In your example, the test needs_i8_conversion(dtype) is False

This doesn't make sense to me. dtype should be np.dtype("M8[ns]"), for which needs_i8_conversion(dtype) is True.

For a starker example, if you pass a tz to the date_range, the ser.unique raises in this branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, the tz example shows that what I thought was dead code can't be removed. Will close this PR then.

return self._shallow_copy(left=new_left, right=new_right)

def unique(self) -> IntervalArray:
Expand Down