-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
allow negative freq strings #8651
Conversation
Just so I understand, this is not backwards-incompatible because we did not allow negative frequency strings earlier? |
Yes that's correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great @mathause—thanks!
I think the tests you updated look pretty solid. In my testing it seems like it just works, but maybe one more you could add would be an explicit test that calling (Edit this is tested in the infer_freq
on a decreasing CFTimeIndex
returns a negative frequency?test_date_range_like
test).
Co-authored-by: Spencer Clark <spencerkclark@gmail.com>
xarray/tests/test_cftime_offsets.py
Outdated
result = date_range(start, end, freq=freq, calendar="standard", use_cftime=True) | ||
expected = date_range(start, end, freq=freq, use_cftime=False) | ||
|
||
# end of month/ year is not convertable to datetimeindex | ||
assert result.size == expected.size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can check for equality now, similar to the test above:
result = date_range(start, end, freq=freq, calendar="standard", use_cftime=True) | |
expected = date_range(start, end, freq=freq, use_cftime=False) | |
# end of month/ year is not convertable to datetimeindex | |
assert result.size == expected.size | |
result = date_range(start, end, freq=freq, calendar="standard", use_cftime=True) | |
result = result.to_datetimeindex() | |
expected = date_range(start, end, freq=freq, use_cftime=False) | |
np.testing.assert_array_equal(result, expected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately I suppose this test could now be merged with the one above—i.e. just merge the frequency strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of course - thanks
whats-new.rst
This allows negative freq strings as discussed in #8627 (comment) Deciding which tests to update was not easy.
The pandas
_generate_range
function was moved to https://github.com/pandas-dev/pandas/blob/3c96b8ff6d399fbec8d4d533e8e8618c592bb64b/pandas/core/arrays/datetimes.py#L2725 They no longerrollback
theend
. I had to remove this as well such that the following are eqivalent:I am slightly nervous about this but all the tests still pass...
Once again cc @spencerkclark