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.

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

Revert inclusive default change of IntervalDtype #47367

Merged
merged 11 commits into from
Jul 6, 2022
Merged

Revert inclusive default change of IntervalDtype #47367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f471159
Revert inclusive change of IntervalDtype
phofl Jun 15, 2022
f35b5cd
Add properties back in and reorder arguments
phofl Jun 16, 2022
9aeeced
Change default
phofl Jun 16, 2022
6f791fd
Add tests
phofl Jun 16, 2022
b2abf87
Fix ci
phofl Jun 16, 2022
2241b27
Merge branch 'main' of https://github.com/pandas-dev/pandas into 47365
phofl Jun 16, 2022
91883d9
Fix another test
phofl Jun 16, 2022
a6c9602
Fix docstring
phofl Jun 16, 2022
728fdd8
Fix typing
phofl Jun 17, 2022
32faa5f
Use decorator
phofl Jun 18, 2022
ad54daa
Merge remote-tracking branch 'upstream/main' into 47365
phofl Jul 2, 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
1 change: 1 addition & 0 deletions doc/source/reference/arrays.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,7 @@ Properties
:toctree: api/

Interval.inclusive
Interval.closed
Interval.closed_left
Interval.closed_right
Interval.is_empty
Expand Down
2 changes: 2 additions & 0 deletions pandas/_libs/interval.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ class Interval(IntervalMixin, Generic[_OrderableT]):
def right(self: Interval[_OrderableT]) -> _OrderableT: ...
@property
def inclusive(self) -> IntervalClosedType: ...
@property
def closed(self) -> IntervalClosedType: ...
mid: _MidDescriptor
length: _LengthDescriptor
def __init__(
Expand Down
21 changes: 19 additions & 2 deletions pandas/_libs/interval.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ from cpython.datetime cimport (
import_datetime,
)

from pandas.util._exceptions import find_stack_level

import_datetime()

cimport cython
Expand Down Expand Up @@ -229,7 +231,7 @@ def _warning_interval(inclusive: str | None = None, closed: None | lib.NoDefault
stacklevel=2,
)
if closed is None:
inclusive = "both"
inclusive = "right"
Copy link
Member

Choose a reason for hiding this comment

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

not sure about some of the handling/validation here but that's outside the scope of this PR.

we have a deprecate_kwarg decorator that caters for renaming keywords. not sure why not used.

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot about it, not sure why it was not used before. Switched over now.

elif closed in ("both", "neither", "left", "right"):
inclusive = closed
else:
Expand Down Expand Up @@ -364,7 +366,7 @@ cdef class Interval(IntervalMixin):
inclusive, closed = _warning_interval(inclusive, closed)

if inclusive is None:
inclusive = "both"
inclusive = "right"

if inclusive not in VALID_CLOSED:
raise ValueError(f"invalid option for 'inclusive': {inclusive}")
Expand All @@ -379,6 +381,21 @@ cdef class Interval(IntervalMixin):
self.right = right
self.inclusive = inclusive

@property
def closed(self):
"""
Whether the interval is closed on the left-side, right-side, both or
neither.

.. deprecated:: 1.5.0
"""
warnings.warn(
"Attribute `closed` is deprecated in favor of `inclusive`.",
FutureWarning,
stacklevel=find_stack_level(),
)
return self.inclusive

def _validate_endpoint(self, endpoint):
# GH 23013
if not (is_integer_object(endpoint) or is_float_object(endpoint) or
Expand Down
2 changes: 1 addition & 1 deletion pandas/_libs/intervaltree.pxi.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ cdef class IntervalTree(IntervalMixin):
inclusive, closed = _warning_interval(inclusive, closed)

if inclusive is None:
inclusive = "both"
inclusive = "right"
Copy link
Member

Choose a reason for hiding this comment

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

is IntervalTree public? (or is the class the return type of a public function/method?)

maybe we don't need the deprecation here or don't even need to change at all.

is your understanding that the change from closed -> inclusive is for the public api or across the whole codebase?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can see IntervalTree is private (There are no docs around). It obviously makes sense to rename the kwarg across the whole code base. But since its private, we don't have to be backwards compatible.

Copy link
Member

Choose a reason for hiding this comment

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

yep. probably ok to remove the deprecation here as a follow-up


if inclusive not in ['left', 'right', 'both', 'neither']:
raise ValueError("invalid option for 'inclusive': %s" % inclusive)
Expand Down
21 changes: 12 additions & 9 deletions pandas/core/arrays/arrow/_arrow_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@
import numpy as np
import pyarrow

from pandas._libs import lib
from pandas._libs.interval import _warning_interval
from pandas.errors import PerformanceWarning
from pandas.util._decorators import deprecate_kwarg
from pandas.util._exceptions import find_stack_level

from pandas.core.arrays.interval import VALID_CLOSED
Expand Down Expand Up @@ -105,15 +104,10 @@ def to_pandas_dtype(self):


class ArrowIntervalType(pyarrow.ExtensionType):
def __init__(
self,
subtype,
inclusive: str | None = None,
closed: None | lib.NoDefault = lib.no_default,
) -> None:
@deprecate_kwarg(old_arg_name="closed", new_arg_name="inclusive")
def __init__(self, subtype, inclusive: str) -> None:
# attributes need to be set first before calling
# super init (as that calls serialize)
inclusive, closed = _warning_interval(inclusive, closed)
assert inclusive in VALID_CLOSED
self._closed = inclusive
if not isinstance(subtype, pyarrow.DataType):
Expand All @@ -131,6 +125,15 @@ def subtype(self):
def inclusive(self):
return self._closed

@property
def closed(self):
warnings.warn(
"Attribute `closed` is deprecated in favor of `inclusive`.",
FutureWarning,
stacklevel=find_stack_level(),
)
return self._closed

def __arrow_ext_serialize__(self):
metadata = {"subtype": str(self.subtype), "inclusive": self.inclusive}
return json.dumps(metadata).encode()
Expand Down
46 changes: 36 additions & 10 deletions pandas/core/arrays/interval.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
cast,
overload,
)
import warnings

import numpy as np

Expand All @@ -25,7 +26,6 @@
VALID_CLOSED,
Interval,
IntervalMixin,
_warning_interval,
intervals_to_interval_bounds,
)
from pandas._libs.missing import NA
Expand All @@ -43,8 +43,10 @@
from pandas.errors import IntCastingNaNError
from pandas.util._decorators import (
Appender,
deprecate_kwarg,
deprecate_nonkeyword_arguments,
)
from pandas.util._exceptions import find_stack_level

from pandas.core.dtypes.cast import LossySetitemError
from pandas.core.dtypes.common import (
Expand Down Expand Up @@ -220,16 +222,15 @@ def ndim(self) -> Literal[1]:
# ---------------------------------------------------------------------
# Constructors

@deprecate_kwarg(old_arg_name="closed", new_arg_name="inclusive")
def __new__(
cls: type[IntervalArrayT],
data,
inclusive: str | None = None,
closed: None | lib.NoDefault = lib.no_default,
dtype: Dtype | None = None,
copy: bool = False,
verify_integrity: bool = True,
):
inclusive, closed = _warning_interval(inclusive, closed)

data = extract_array(data, extract_numpy=True)

Expand Down Expand Up @@ -267,24 +268,22 @@ def __new__(
)

@classmethod
@deprecate_kwarg(old_arg_name="closed", new_arg_name="inclusive")
def _simple_new(
cls: type[IntervalArrayT],
left,
right,
inclusive=None,
closed: None | lib.NoDefault = lib.no_default,
copy: bool = False,
dtype: Dtype | None = None,
verify_integrity: bool = True,
) -> IntervalArrayT:
result = IntervalMixin.__new__(cls)

inclusive, closed = _warning_interval(inclusive, closed)

if inclusive is None and isinstance(dtype, IntervalDtype):
inclusive = dtype.inclusive

inclusive = inclusive or "both"
inclusive = inclusive or "right"

left = ensure_index(left, copy=copy)
right = ensure_index(right, copy=copy)
Expand Down Expand Up @@ -424,13 +423,17 @@ def _from_factorized(
),
}
)
@deprecate_kwarg(old_arg_name="closed", new_arg_name="inclusive")
def from_breaks(
cls: type[IntervalArrayT],
breaks,
inclusive="both",
inclusive: IntervalClosedType | None = None,
copy: bool = False,
dtype: Dtype | None = None,
) -> IntervalArrayT:
if inclusive is None:
inclusive = "right"

breaks = _maybe_convert_platform_interval(breaks)

return cls.from_arrays(
Expand Down Expand Up @@ -501,14 +504,19 @@ def from_breaks(
),
}
)
@deprecate_kwarg(old_arg_name="closed", new_arg_name="inclusive")
def from_arrays(
cls: type[IntervalArrayT],
left,
right,
inclusive="both",
inclusive: IntervalClosedType | None = None,
copy: bool = False,
dtype: Dtype | None = None,
) -> IntervalArrayT:

if inclusive is None:
inclusive = "right"

left = _maybe_convert_platform_interval(left)
right = _maybe_convert_platform_interval(right)

Expand Down Expand Up @@ -570,13 +578,17 @@ def from_arrays(
),
}
)
@deprecate_kwarg(old_arg_name="closed", new_arg_name="inclusive")
def from_tuples(
cls: type[IntervalArrayT],
data,
inclusive="both",
inclusive=None,
copy: bool = False,
dtype: Dtype | None = None,
) -> IntervalArrayT:
if inclusive is None:
inclusive = "right"

if len(data):
left, right = [], []
else:
Expand Down Expand Up @@ -1355,6 +1367,19 @@ def inclusive(self) -> IntervalClosedType:
"""
return self.dtype.inclusive

@property
def closed(self) -> IntervalClosedType:
"""
Whether the intervals are closed on the left-side, right-side, both or
neither.
"""
warnings.warn(
"Attribute `closed` is deprecated in favor of `inclusive`.",
FutureWarning,
stacklevel=find_stack_level(),
)
return self.dtype.inclusive

_interval_shared_docs["set_closed"] = textwrap.dedent(
"""
Return an %(klass)s identical to the current one, but closed on the
Expand Down Expand Up @@ -1395,6 +1420,7 @@ def inclusive(self) -> IntervalClosedType:
),
}
)
@deprecate_kwarg(old_arg_name="closed", new_arg_name="inclusive")
def set_closed(
self: IntervalArrayT, inclusive: IntervalClosedType
) -> IntervalArrayT:
Expand Down
11 changes: 11 additions & 0 deletions pandas/core/dtypes/dtypes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
MutableMapping,
cast,
)
import warnings

import numpy as np
import pytz
Expand Down Expand Up @@ -40,6 +41,7 @@
npt,
type_t,
)
from pandas.util._exceptions import find_stack_level

from pandas.core.dtypes.base import (
ExtensionDtype,
Expand Down Expand Up @@ -1176,6 +1178,15 @@ def _can_hold_na(self) -> bool:
def inclusive(self):
return self._closed

@property
def closed(self):
warnings.warn(
"Attribute `closed` is deprecated in favor of `inclusive`.",
FutureWarning,
stacklevel=find_stack_level(),
)
return self._closed

@property
def subtype(self):
"""
Expand Down
Loading