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 9 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
9 changes: 9 additions & 0 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 @@ -131,6 +131,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
91 changes: 85 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 @@ -14,6 +14,7 @@
cast,
overload,
)
import warnings

import numpy as np

Expand Down Expand Up @@ -44,6 +45,7 @@
Appender,
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,10 +222,10 @@ 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,
closed: None | lib.NoDefault = lib.no_default,
):
inclusive, closed = _warning_interval(inclusive, closed)

Expand Down Expand Up @@ -280,7 +282,7 @@ def _simple_new(
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 @@ -423,10 +425,16 @@ def _from_factorized(
def from_breaks(
cls: type[IntervalArrayT],
breaks,
inclusive="both",
inclusive: IntervalClosedType | None = lib.no_default, # type: ignore[assignment] # noqa: E501
copy: bool = False,
dtype: Dtype | None = None,
closed: IntervalClosedType | None = lib.no_default, # type: ignore[assignment]
) -> IntervalArrayT:
# Incompatible default for argument "closed" (default has type
# "Literal[_NoDefault.no_default]", argument has type
# "Optional[Union[Literal['left', 'right'], Literal['both', 'neither']]]")
inclusive = _warning_interval_array_functions(inclusive, closed)

breaks = _maybe_convert_platform_interval(breaks)

return cls.from_arrays(
Expand Down Expand Up @@ -501,10 +509,16 @@ def from_arrays(
cls: type[IntervalArrayT],
left,
right,
inclusive="both",
inclusive: IntervalClosedType | None = lib.no_default, # type: ignore[assignment] # noqa: E501
copy: bool = False,
dtype: Dtype | None = None,
closed: IntervalClosedType | None = lib.no_default, # type: ignore[assignment]
) -> IntervalArrayT:
# Incompatible default for argument "closed" (default has type
# "Literal[_NoDefault.no_default]", argument has type
# "Optional[Union[Literal['left', 'right'], Literal['both', 'neither']]]")
inclusive = _warning_interval_array_functions(inclusive, closed)

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

Expand Down Expand Up @@ -569,10 +583,12 @@ def from_arrays(
def from_tuples(
cls: type[IntervalArrayT],
data,
inclusive="both",
inclusive=lib.no_default,
copy: bool = False,
dtype: Dtype | None = None,
closed=lib.no_default,
) -> IntervalArrayT:
inclusive = _warning_interval_array_functions(inclusive, closed)
if len(data):
left, right = [], []
else:
Expand Down Expand Up @@ -1351,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 @@ -1392,8 +1421,36 @@ def inclusive(self) -> IntervalClosedType:
}
)
def set_closed(
self: IntervalArrayT, inclusive: IntervalClosedType
self: IntervalArrayT,
inclusive: IntervalClosedType = lib.no_default, # type: ignore[assignment]
closed: IntervalClosedType = lib.no_default, # type: ignore[assignment]
) -> IntervalArrayT:
# Incompatible default for argument "closed" (default has type
# "Literal[_NoDefault.no_default]", argument has type
# "Optional[Union[Literal['left', 'right'], Literal['both', 'neither']]]")
# Non-overlapping equality check (left operand type:
# "Union[Literal['left', 'right'], Literal['both', 'neither']]",
# right operand type: "Literal[_NoDefault.no_default]")
if (
inclusive == lib.no_default # type: ignore[comparison-overlap]
and closed == lib.no_default
):
raise ValueError("inclusive has to be passed into the function.")

elif (
closed != lib.no_default # type: ignore[comparison-overlap]
and inclusive != lib.no_default # type: ignore[comparison-overlap]
):
raise ValueError("You can not pass inclusive and closed.")

elif closed != lib.no_default:
warnings.warn(
"Argument closed is deprecated, use inclusive instead.",
FutureWarning,
stacklevel=find_stack_level(),
)
inclusive = closed

if inclusive not in VALID_CLOSED:
msg = f"invalid option for 'inclusive': {inclusive}"
raise ValueError(msg)
Expand Down Expand Up @@ -1757,3 +1814,25 @@ def _maybe_convert_platform_interval(values) -> ArrayLike:
if not hasattr(values, "dtype"):
return np.asarray(values)
return values


def _warning_interval_array_functions(inclusive, closed):
"""
warning in interval class for variable inclusive and closed
"""
if inclusive != lib.no_default and closed != lib.no_default:
raise ValueError(
"Deprecated argument `closed` cannot be passed "
"if argument `inclusive` is passed"
)
elif closed != lib.no_default:
warnings.warn(
"Argument `closed` is deprecated in favor of `inclusive`.",
FutureWarning,
stacklevel=find_stack_level(),
)
inclusive = closed
elif inclusive == lib.no_default:
inclusive = "right"

return inclusive
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