Revert inclusive default change of IntervalDtype #47367
Conversation
phofl
changed the title
|
I've moved closed to the end of the arguments, this means the previous position of closed is now filled by inclusive, e.g. positional arguments just work and if something is using keyword arguments, it will show the warning but not disturb the previous order of arguments. Added propeties for closed and deprecated them @simonjayhawkins Coud you have a look? Things still to do (but as follow up):
|
| @@ -69,7 +69,7 @@ cdef class IntervalTree(IntervalMixin): | |||
| inclusive, closed = _warning_interval(inclusive, closed) | |||
|
|
|||
| if inclusive is None: | |||
| inclusive = "both" | |||
| inclusive = "right" | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yep. probably ok to remove the deprecation here as a follow-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" | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I forgot about it, not sure why it was not used before. Switched over now.
|
this looks fine ex the comments |
There was a problem hiding this comment.
Thanks @phofl generally lgtm.
is it possible that _warning_interval can be removed entirely as a follow-up?
not sure if can be done easily but can deprecate_kwarg be used without allowing None as an an acceptable type and just set the default? (not this PR) (so that the deprecation is just removing the decorator)
also some duplication for renaming attribute. maybe some sort of deprecate_attr possible? (not this PR)
|
Will have a look. I think if we remove the deprecation for IntervallTree, we should be able to remove Edit: Will probably put up a few clean up prs separately to keep them easy to review |
…48116) * Revert "Cln tests interval wrt inclusive (#47775)" This reverts commit 2d6e0b2. * Revert "CLN: Rename private variables to inclusive (#47655)" This reverts commit 102b3ca. * Revert "TYP: Improve typing interval inclusive (#47646)" This reverts commit 5506476. * Revert "DEPR: Deprecate set_closed and add set_incluive (#47636)" This reverts commit bd4ff39. * Revert "DEPR: Remove deprecation from private class IntervalTree (#47637)" This reverts commit f6658ef. * Revert "Revert inclusive default change of IntervalDtype (#47367)" This reverts commit d9dd128. * Revert "ENH: consistency of input args for boundaries - Interval (#46522)" This reverts commit 7e23a37. * Revert "ENH: consistency of input args for boundaries - pd.interval_range (#46355)" This reverts commit 073b353. * Fix ArrowIntervalType manually * Remove unused import * Fix doctest and leftover usage * Fix remaining tests * Fix wording in doctoring Co-authored-by: Patrick Hoefler <61934744+phofl@users.noreply.github.com>
…andas-dev#48116) * Revert "Cln tests interval wrt inclusive (pandas-dev#47775)" This reverts commit 2d6e0b2. * Revert "CLN: Rename private variables to inclusive (pandas-dev#47655)" This reverts commit 102b3ca. * Revert "TYP: Improve typing interval inclusive (pandas-dev#47646)" This reverts commit 5506476. * Revert "DEPR: Deprecate set_closed and add set_incluive (pandas-dev#47636)" This reverts commit bd4ff39. * Revert "DEPR: Remove deprecation from private class IntervalTree (pandas-dev#47637)" This reverts commit f6658ef. * Revert "Revert inclusive default change of IntervalDtype (pandas-dev#47367)" This reverts commit d9dd128. * Revert "ENH: consistency of input args for boundaries - Interval (pandas-dev#46522)" This reverts commit 7e23a37. * Revert "ENH: consistency of input args for boundaries - pd.interval_range (pandas-dev#46355)" This reverts commit 073b353. * Fix ArrowIntervalType manually * Remove unused import * Fix doctest and leftover usage * Fix remaining tests * Fix wording in doctoring Co-authored-by: Patrick Hoefler <61934744+phofl@users.noreply.github.com>
This adresses the changed default, will add the attribute back in as a follow up