ENH: consistency of input args for boundaries (pd.date_range) by zyc09 · Pull Request #43504 · pandas-dev/pandas

zyc09 · 2021-09-10T20:35:29Z

tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

pandas/core/arrays/datetimelike.py

pandas/core/indexes/datetimes.py

attack68 · 2021-09-12T18:55:03Z

pandas/core/arrays/datetimelike.py

    return periods


+def validate_inclusiveness(inclusive):


maybe just call this validate_inclusive?

couple of things. I would like to move this new routine & validate_entpoint to pandas/core/indexers/utils.py i think.
Also need to slightly refactor .between_time (which was just merged) to use the same.

Might be best to do these as a pre-cursor PR to this one.

might also look at other merged prs from underlying issue that may be able to use this validator,

jreback · 2021-09-12T19:29:08Z

pandas/core/arrays/datetimelike.py

    return periods


+def validate_inclusiveness(inclusive):


couple of things. I would like to move this new routine & validate_entpoint to pandas/core/indexers/utils.py i think.
Also need to slightly refactor .between_time (which was just merged) to use the same.

Might be best to do these as a pre-cursor PR to this one.

jreback · 2021-09-12T19:32:06Z

pandas/tests/indexes/datetimes/test_date_range.py

-        closed = date_range(begin, end, closed=None, freq=freq)
-        left = date_range(begin, end, closed="left", freq=freq)
-        right = date_range(begin, end, closed="right", freq=freq)
+        both = date_range(begin, end, inclusive="both", freq=freq)


can you parameterize this test instead, using the inclusive_endpoints_fixture fixtures (its in pandas/tests/frameso likely need to move topandas/conftest.py`

yea makes sense, will do that

jreback · 2021-09-12T19:32:18Z

pandas/tests/indexes/datetimes/test_date_range.py

-            closed = date_range(begin, end, closed=None, freq=freq)
-            left = date_range(begin, end, closed="left", freq=freq)
-            right = date_range(begin, end, closed="right", freq=freq)
+            both = date_range(begin, end, inclusive="both", freq=freq)


jreback · 2021-09-12T19:32:25Z

pandas/tests/indexes/datetimes/test_date_range.py

-            closed = date_range(begin, end, closed=None, freq=freq, tz="US/Eastern")
-            left = date_range(begin, end, closed="left", freq=freq, tz="US/Eastern")
-            right = date_range(begin, end, closed="right", freq=freq, tz="US/Eastern")
+            both = date_range(begin, end, inclusive="both", freq=freq, tz="US/Eastern")


jreback · 2021-09-12T19:32:30Z

pandas/tests/indexes/datetimes/test_date_range.py


-    @pytest.mark.parametrize("closed", ["right", "left", None])
-    def test_range_closed_boundary(self, closed):
+    @pytest.mark.parametrize("inclusive", ["right", "left", "both", "neither"])


pandas/tests/indexes/datetimes/test_date_range.py

jreback · 2021-09-12T19:33:36Z

is this also closing #43394 ? e.g. a bug fix, if so this needs a separate note

zyc09 · 2021-09-13T11:18:26Z

is this also closing #43394 ? e.g. a bug fix, if so this needs a separate note

What do you mean by separate note by the way?

jreback · 2021-09-14T14:10:21Z

go ahead and rebase this

jreback · 2021-09-14T16:12:24Z

pandas/core/arrays/datetimes.py

+        # and removing would leave index empty
+        to_remove_any = not (
+            (left_inclusive or right_inclusive)
+            and len(index) == 1


shouldn't this be len(index) ?

mm yeah, changed that and changed the tests in test_date_range

jreback · 2021-09-14T18:19:51Z

pandas/conftest.py

    return request.param


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


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

jreback

looking good a few comments

jreback · 2021-09-14T18:20:09Z

pandas/core/indexes/datetimes.py

+           Argument `closed` have been deprecated to standardize boundary inputs.
+           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.


versionadded 1.4.0

jreback · 2021-09-14T18:21:06Z

pandas/tests/indexes/datetimes/test_date_range.py

+        left_match = begin == both_range[0]
+        right_match = end == both_range[-1]
+
+        if inclusive_endpoints_fixture == "left" and right_match:


consider making this a function and share among these tests

I defined the function on the top of the test_date_range.py file, does it belong elsewhere?

jreback · 2021-09-14T18:21:17Z

pandas/tests/indexes/datetimes/test_date_range.py

+        right_match = endtz == both_range[-1]
+
+        if inclusive_endpoints_fixture == "left" and right_match:
+            expected_range = both_range[:-1]


same (so we don't repeat ourselves)

attack68 · 2021-09-14T20:51:13Z

pandas/core/arrays/datetimes.py

+            (left_inclusive or right_inclusive)
+            and len(index)
+            and start == index[0]
+            and start == end


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?

attack68 · 2021-09-14T20:55:13Z

pandas/core/arrays/datetimes.py

+        )
+
+        if to_remove_any:
+            if (not left_inclusive) and len(index) and index[0] == start:


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?

I refactored the logic a little and I am not sure if it is any better though

jreback

lgttm. @attack68 if comments

jreback · 2021-09-15T16:30:11Z

pandas/core/indexes/datetimes.py

    DatetimeIndex(['2017-01-02', '2017-01-03', '2017-01-04'],
                  dtype='datetime64[ns]', freq='D')
    """
+    if inclusive is not None and not isinstance(closed, lib.NoDefault):


its possible we would want to share this (across functoions), but ok here

jreback · 2021-09-15T16:50:20Z

https://github.com/pandas-dev/pandas/pull/43504/checks?check_run_id=3612446736

couple of tests that need addressing (you can either assert the future warning or just change closed -> inclusive in the test)

attack68

lgtm pending green tests and some grammar corrections.

attack68 · 2021-09-15T17:49:39Z

pandas/core/indexes/datetimes.py

        the 'left', 'right', or both sides (None, the default).
+
+        .. deprecated:: 1.4.0
+           Argument `closed` have been deprecated to standardize boundary inputs.


'has' not 'have'.

attack68 · 2021-09-15T17:50:32Z

pandas/core/indexes/datetimes.py

        the 'left', 'right', or both sides (None).
+
+        .. deprecated:: 1.4.0
+           Argument `closed` have been deprecated to standardize boundary inputs.


jreback · 2021-09-28T19:01:40Z

some comments & can you merge master. ping on green.

attack68 · 2021-10-03T16:44:08Z

thanks @zyc09

…-dev#43504) Co-authored-by: JHM Darbyshire <24256554+attack68@users.noreply.github.com>

attack68 reviewed Sep 11, 2021

View reviewed changes

pandas/core/arrays/datetimelike.py Outdated Show resolved Hide resolved

attack68 reviewed Sep 11, 2021

View reviewed changes

pandas/core/indexes/datetimes.py Outdated Show resolved Hide resolved

attack68 reviewed Sep 12, 2021

View reviewed changes

jreback added the Enhancement label Sep 12, 2021

jreback mentioned this pull request Sep 12, 2021

DEPR: log of deprecations in 1.x (to be removed in 2.0) #30228

Closed

jreback requested changes Sep 12, 2021

View reviewed changes

jreback added Deprecate Functionality to remove in pandas Datetime Datetime data dtype labels Sep 12, 2021

jreback mentioned this pull request Sep 12, 2021

ENH: consistency of input args for boundaries #40245

Open

10 tasks

zyc09 force-pushed the bug/date-range branch from d84b4ae to ff9b329 Compare September 14, 2021 15:11

Change date_range inclusive

203733e

zyc09 force-pushed the bug/date-range branch from ff9b329 to 203733e Compare September 14, 2021 15:26

zyc09 added 3 commits September 14, 2021 23:32

to use validate_inclusive in util

05def22

typing + docstring

be085dd

Move inclusive_endpoints_fixture to outer conftest

ff0649c

jreback requested changes Sep 14, 2021

View reviewed changes

zyc09 added 2 commits September 15, 2021 01:52

Rework Tests

67015ff

remove line

e80329f

jreback added this to the 1.4 milestone Sep 14, 2021

jreback reviewed Sep 14, 2021

View reviewed changes

jreback requested changes Sep 14, 2021

View reviewed changes

attack68 reviewed Sep 14, 2021

View reviewed changes

Amend logic + factoring of tests

245c50e

jreback approved these changes Sep 15, 2021

View reviewed changes

closed -> inclusive in tests

2c5d65d

attack68 approved these changes Sep 15, 2021

View reviewed changes

zyc09 and others added 2 commits September 16, 2021 02:04

docstring grammar

4b85743

Merge branch 'master' into bug/date-range

eea0075

attack68 merged commit 3db765a into pandas-dev:master Oct 3, 2021

gasparitiago pushed a commit to gasparitiago/pandas that referenced this pull request Oct 9, 2021

ENH: consistency of input args for boundaries (pd.date_range) (pandas…

0cae478

…-dev#43504) Co-authored-by: JHM Darbyshire <24256554+attack68@users.noreply.github.com>

mroeschke mentioned this pull request Nov 9, 2022

DEPR: Enforce deprecations in indexes/datetimes.py #49607

Merged

		return request.param


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

Uh oh!

Comments

Conversation

zyc09 commented Sep 10, 2021 • edited by jreback Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jreback commented Sep 12, 2021

Uh oh!

zyc09 commented Sep 13, 2021

Uh oh!

jreback commented Sep 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback commented Sep 15, 2021

Uh oh!

attack68 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback commented Sep 28, 2021

Uh oh!

attack68 commented Oct 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

zyc09 commented Sep 10, 2021 •

edited by jreback

Loading