BUG: date_range inclusive parameter behavior doesn't match interval notation
#55319
Conversation
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
|
This pr is ready for review |
There was a problem hiding this comment.
Thanks for the contribution @PiotrekB416, and sorry it too so long to review, seems like this PR was forgotten in the long list of PRs we have.
It's a bit difficult to understand all possible cases reading the diff, and I'm worried that by fixing the bug we may be introducing new ones.
What I'd like to see in general is that you keep existing tests as they are (in general we shouldn't have wrong cases as tests), and that you add one new test that it reproduces the fixed issue.
In this case the test seems a bit cumbersome, was it asserting cases with the behavior we consider a bug in this PR?
@MarcoGorelli do you have an opinion here?
Yes, as far as I could tell the tests had a few mistakes in them, also they were written in a really strange and unnecessarily complex way. expected = date_range(bday_start, bday_end, freq="D")should get the expected range, but the |
|
Thanks for the feedback and the work on this @PiotrekB416 Marco will be able to provide better feedback when he has the time to review, but what makes sense to me is:
While what you are doing here seems reasonable, it feels strange that the complex logic you are making much simpler here wasn't implemented for a reason, and that we may be missing some use case where your changes fix a bug, but introduce a new one. Changing too much here makes difficult to see if that will be the case. I feel like the approach I'm proposing should minimize the chances of it happening. What do you think? |
| begintz = Timestamp("2011/1/1", tz="US/Eastern") | ||
| endtz = Timestamp("2014/1/1", tz="US/Eastern") | ||
| # begintz = Timestamp("2011/1/1", tz="US/Eastern") | ||
| # endtz = Timestamp("2014/1/1", tz="US/Eastern") |
There was a problem hiding this comment.
why is this part of the test comment out?
There was a problem hiding this comment.
The shortest answer would be that those lines are not needed and can be safely deleted. I just commented them out when I was testing and then forgot to remove them.
And as for why they were there in the first place. They were used by _get_expected_range, but they are not needed for the function to work
There was a problem hiding this comment.
ok thanks - if they're not needed, then to simplify reviews, could you please first open a precursor PR in which you remove anything that's not needed, and then we keep this PR focused on the logic changes (+ relevant tests)? thanks 🙏
There was a problem hiding this comment.
I'm afraid that might not be possible. The code is complicated because of its logic errors, thus I can't simplify it without making logic changes.
For example:
in test_range_closed_boundary
expected_right = both_boundary[1:]
expected_left = both_boundary[:-1]
expected_both = both_boundary[:]
expected_neither = both_boundary[1:-1]this simplification doesn't work if there are no logic changes in the _generate_range function, because then the expected values are compared to incorrectly generated ranges.
|
Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen. |
closes BUG:
date_rangeinclusiveparameter behavior doesn't match interval notation whenstart == end#55293closes BUG: the behavior of "date_range" function with "periods & inclusive" arguments #46331
Added an entry in the latest
doc/source/whatsnew/v2.1.3.rstfile if fixing a bug or adding a new feature.This pr fixes the issues with
inclusiveindate_rangeGot rid of the problematic expression which seems to be
i8values[] == start/end_i8, also theandin line 498 should have been anor. However after those fixes the if-statement was over complicated, so I simplified it.