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.

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

ENH: consistency of input args for boundaries (pd.date_range) #43504

Merged
merged 10 commits into from
Oct 3, 2021
Merged
2 changes: 2 additions & 0 deletions doc/source/whatsnew/v1.4.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ Other Deprecations
- Deprecated the ``squeeze`` argument to :meth:`read_csv`, :meth:`read_table`, and :meth:`read_excel`. Users should squeeze the DataFrame afterwards with ``.squeeze("columns")`` instead. (:issue:`43242`)
- Deprecated the ``index`` argument to :class:`SparseArray` construction (:issue:`23089`)
-
- Deprecated the ``closed`` argument in :meth:`date_range` and :meth:`bdate_range` in favor of ``inclusive`` argument; In a future version passing ``closed`` will raise (:issue:`40245`)

.. ---------------------------------------------------------------------------

Expand Down Expand Up @@ -340,6 +341,7 @@ Datetimelike
^^^^^^^^^^^^
- Bug in :class:`DataFrame` constructor unnecessarily copying non-datetimelike 2D object arrays (:issue:`39272`)
- :func:`to_datetime` would silently swap ``MM/DD/YYYY`` and ``DD/MM/YYYY`` formats if the given ``dayfirst`` option could not be respected - now, a warning is raised in the case of delimited date strings (e.g. ``31-12-2012``) (:issue:`12585`)
- Bug in :meth:`date_range` and :meth:`bdate_range` do not return right bound when ``start`` = ``end`` and set is closed on one side (:issue:`43394`)
-

Timedelta
Expand Down
5 changes: 5 additions & 0 deletions pandas/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,11 @@ def nselect_method(request):
return request.param


@pytest.fixture(params=["both", "neither", "left", "right"])
Copy link
Contributor

Choose a reason for hiding this comment

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

can you put next to this one and add a doc-string that is indicative

@pytest.fixture(params=["left", "right", "both", "neither"])
def closed(request):
    """
    Fixture for trying all interval closed parameters.
    """
    return request.param

def inclusive_endpoints_fixture(request):
return request.param


# ----------------------------------------------------------------
# Missing values & co.
# ----------------------------------------------------------------
Expand Down
24 changes: 17 additions & 7 deletions pandas/core/arrays/datetimes.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
)
from pandas._typing import npt
from pandas.errors import PerformanceWarning
from pandas.util._validators import validate_endpoints
from pandas.util._validators import validate_inclusive

from pandas.core.dtypes.cast import astype_dt64_to_dt64tz
from pandas.core.dtypes.common import (
Expand Down Expand Up @@ -394,7 +394,7 @@ def _generate_range(
normalize=False,
ambiguous="raise",
nonexistent="raise",
closed=None,
inclusive="both",
):

periods = dtl.validate_periods(periods)
Expand All @@ -417,7 +417,7 @@ def _generate_range(
if start is NaT or end is NaT:
raise ValueError("Neither `start` nor `end` can be NaT")

left_closed, right_closed = validate_endpoints(closed)
left_inclusive, right_inclusive = validate_inclusive(inclusive)
start, end, _normalized = _maybe_normalize_endpoints(start, end, normalize)
tz = _infer_tz_from_endpoints(start, end, tz)

Expand Down Expand Up @@ -477,10 +477,20 @@ def _generate_range(
arr = arr.astype("M8[ns]", copy=False)
index = cls._simple_new(arr, freq=None, dtype=dtype)

if not left_closed and len(index) and index[0] == start:
index = index[1:]
if not right_closed and len(index) and index[-1] == end:
index = index[:-1]
# do not remove when one side is inclusive
# and removing would leave index empty
to_remove_any = not (
(left_inclusive or right_inclusive)
and len(index)
and start == index[0]
and start == end
Copy link
Contributor

Choose a reason for hiding this comment

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

when is start != index[0]?
when is len(index) == 0 i.e. when will len(index) resolve as False?
when is start == end?
Do any of these cases imply the others?

)

if to_remove_any:
if (not left_inclusive) and len(index) and index[0] == start:
Copy link
Contributor

Choose a reason for hiding this comment

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

you are reusing the same logic tests deriving to_remove_any. I haven't come up with a better way but instinctively this feels a complicated logic path. Have you tried refactoring to a simpler setup?

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 refactored the logic a little and I am not sure if it is any better though

index = index[1:]
if (not right_inclusive) and len(index) and index[-1] == end:
index = index[:-1]

dtype = tz_to_dtype(tz)
return cls._simple_new(index._ndarray, freq=freq, dtype=dtype)
Expand Down
43 changes: 40 additions & 3 deletions pandas/core/indexes/datetimes.py
Original file line number Diff line number Diff line change
Expand Up @@ -881,7 +881,8 @@ def date_range(
tz=None,
normalize: bool = False,
name: Hashable = None,
closed=None,
closed: str | None | lib.NoDefault = lib.no_default,
inclusive: str | None = None,
**kwargs,
) -> DatetimeIndex:
"""
Expand Down Expand Up @@ -919,6 +920,12 @@ def date_range(
closed : {None, 'left', 'right'}, optional
Make the interval closed with respect to the given frequency to
the 'left', 'right', or both sides (None, the default).

.. deprecated:: 1.4.0
Argument `closed` have been deprecated to standardize boundary inputs.
Copy link
Contributor

Choose a reason for hiding this comment

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

'has' not 'have'.

Use `inclusive` instead, to set each bound as closed or open.
inclusive : {"both", "neither", "left", "right"}, default "both"
Include boundaries; Whether to set each bound as closed or open.
Copy link
Contributor

Choose a reason for hiding this comment

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

versionadded 1.4.0

**kwargs
For compatibility. Has no effect on the result.

Expand Down Expand Up @@ -1029,6 +1036,28 @@ def date_range(
DatetimeIndex(['2017-01-02', '2017-01-03', '2017-01-04'],
dtype='datetime64[ns]', freq='D')
"""
if inclusive is not None and closed is not lib.no_default:
raise ValueError(
"Deprecated argument `closed` cannot be passed"
"if argument `inclusive` is not None"
)
elif closed is not lib.no_default:
warnings.warn(
"Argument `closed` is deprecated in favor of `inclusive`.",
FutureWarning,
stacklevel=2,
)
if closed is None:
inclusive = "both"
elif closed in ("left", "right"):
inclusive = closed
else:
raise ValueError(
"Argument `closed` has to be either 'left', 'right' or None"
)
elif inclusive is None:
inclusive = "both"

if freq is None and com.any_none(periods, start, end):
freq = "D"

Expand All @@ -1039,7 +1068,7 @@ def date_range(
freq=freq,
tz=tz,
normalize=normalize,
closed=closed,
inclusive=inclusive,
**kwargs,
)
return DatetimeIndex._simple_new(dtarr, name=name)
Expand All @@ -1055,7 +1084,8 @@ def bdate_range(
name: Hashable = None,
weekmask=None,
holidays=None,
closed=None,
closed: lib.NoDefault = lib.no_default,
inclusive: str | None = None,
**kwargs,
) -> DatetimeIndex:
"""
Expand Down Expand Up @@ -1090,6 +1120,12 @@ def bdate_range(
closed : str, default None
Make the interval closed with respect to the given frequency to
the 'left', 'right', or both sides (None).

.. deprecated:: 1.4.0
Argument `closed` have been deprecated to standardize boundary inputs.
Copy link
Contributor

Choose a reason for hiding this comment

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

'has'

Use `inclusive` instead, to set each bound as closed or open.
inclusive : {"both", "neither", "left", "right"}, default "both"
Include boundaries; Whether to set each bound as closed or open.
**kwargs
For compatibility. Has no effect on the result.

Expand Down Expand Up @@ -1143,6 +1179,7 @@ def bdate_range(
normalize=normalize,
name=name,
closed=closed,
inclusive=inclusive,
**kwargs,
)

Expand Down
5 changes: 0 additions & 5 deletions pandas/tests/frame/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,6 @@
import pandas._testing as tm


@pytest.fixture(params=["both", "neither", "left", "right"])
def inclusive_endpoints_fixture(request):
return request.param


@pytest.fixture
def float_frame_with_na():
"""
Expand Down