Skip to content

Quarter offset support for cftime #2702

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

Closed
wants to merge 86 commits into from

Conversation

jwenfai
Copy link
Contributor

@jwenfai jwenfai commented Jan 24, 2019

Waiting for reviews before editing whats-new.rst.

jwenfai and others added 30 commits November 9, 2018 16:58
… 2018-12-05 @max-sixty GitHub reviews for resample-v2-clean pull request). Not cleaned.
Get PEP8 changes from Ouranosinc.
…-v2-clean

# Conflicts:
#	xarray/tests/test_cftimeindex_resample.py
… 2018-12-05 @max-sixty GitHub reviews for resample-v2-clean pull request). Cleaned.
# Conflicts:
#	xarray/core/resample.py
#	xarray/tests/test_cftimeindex_resample.py
Updating to latest xarray.
…a() in groupby.py. Pandas cannot drop indices made up of CFTime objects, so integer indices were swapped in for dropping then swapped back out once NAs are dropped.
…after more tests. 5578 out of 5920 tests passed. Pre-cleaning.
jwenfai and others added 24 commits January 27, 2019 13:45
… 2018-12-05 @max-sixty GitHub reviews for resample-v2-clean pull request). Not cleaned.
… 2018-12-05 @max-sixty GitHub reviews for resample-v2-clean pull request). Cleaned.
…a() in groupby.py. Pandas cannot drop indices made up of CFTime objects, so integer indices were swapped in for dropping then swapped back out once NAs are dropped.
…after more tests. 5578 out of 5920 tests passed. Pre-cleaning.
…after more tests. 5578 out of 5920 tests passed. Cleaned.
…n common.py. Cleaned up print statements. Modified tests that were written under the assumption that CFTimeIndex cannot be resampled so that the tests now pass.
…ex.py. Removed unused code from resample_cftime.py. Removed tests that raise error when resampling CFTimeIndex. Removed temp files.
…range is off is the month of start date % 3 != 0
…sets and resampling quarter offsets. Default values added to CFTimeGrouper.
…sets and resampling quarter offsets. Default values added to CFTimeGrouper.
@jwenfai
Copy link
Contributor Author

jwenfai commented Jan 27, 2019

I rebased, seems like the failing checks are the same as the ones affecting the resample pull request.

@spencerkclark
Copy link
Member

Sorry @jwenfai, things still don't look quite right (we should only see commits related to the addition of quarterly offsets here). If it's easier, feel free to start fresh from a new PR.

seems like the failing checks are the same as the ones affecting the resample pull request.

As @shoyer mentioned in #2593, the tests should be fixed if you merge the latest master branch, now that #2720 has been merged.


@pytest.mark.parametrize('freq', [
'2M', '3M',
'2Q-JUN', '3Q-JUL',
Copy link
Member

@spencerkclark spencerkclark Jan 27, 2019

Choose a reason for hiding this comment

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

Ah, I see you've made some changes related to resampling with quarterly frequencies here. My apologies, I didn't notice that before. That's great, but it would be nice to hold off on that until #2593 has been officially merged. It looks like most of what you're looking to add in this PR is independent of resampling (you only need to update the definition of is_super_daily and how the default arguments are defined to support resampling with quarterly frequencies, right?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the changes are indeed not resample related, except for is_super_daily in get_time_bins and similar code in CFTimeGrouper.

I'll start a new pull request that doesn't touch any of the resample code.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds great, thanks -- that will make things easier to review 👍. Sorry for my confusion here.

@jwenfai jwenfai closed this Jan 27, 2019
@jwenfai jwenfai deleted the quarter-offset branch January 27, 2019 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants