Skip to content
forked from pydata/xarray
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

Add tests for SeasonGrouper API (PR #9524) #40

Merged
merged 2 commits into from
Nov 20, 2024

Conversation

tomvothecoder
Copy link

@tomvothecoder tomvothecoder commented Nov 18, 2024

This PR adds tests for pydata#9524.

@dcherian
Copy link
Owner

Amazing! I am so happy to see this. Are you planning to push more tests or shall I merge?

@tomvothecoder
Copy link
Author

Amazing! I am so happy to see this. Are you planning to push more tests or shall I merge?

Happy to contribute. I'll see if I can come up with more tests then I'll mark it ready for review.

@dcherian
Copy link
Owner

As an aside, if you see scope for logic or code improvements, those suggestions are welcome too. I tried to look at xcdats implementation but couldn't follow it.

@tomvothecoder tomvothecoder changed the title Add tests for SeasonalGrouper API Add tests for SeasonGrouper API (PR #9524) Nov 20, 2024
@tomvothecoder tomvothecoder marked this pull request as ready for review November 20, 2024 17:43
Comment on lines +3340 to +3346
@pytest.mark.xfail
@pytest.mark.parametrize("calendar", _ALL_CALENDARS)
def test_season_grouper_with_months_spanning_calendar_year_using_previous_year(
self, calendar
):
# NOTE: This feature is not implemented yet. Maybe it can be a
# parameter to the `SeasonGrouper` API (e.g. `use_previous_year=True`).
Copy link
Author

Choose a reason for hiding this comment

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

Added this test marked with xfail. I will try to see how we can implement this feature with the SeasonGrouper in another PR. It probably involves more work with the groupby API itself since it uses months from the same year for a season.

@tomvothecoder
Copy link
Author

Hey @dcherian this PR is ready for review and merge. If I have any logic or code improvements, I will open a separate PR or suggest them in pydata#9524.

@dcherian
Copy link
Owner

Thanks, this really helps!

@dcherian dcherian merged commit b385532 into dcherian:custom-groupers Nov 20, 2024
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.

2 participants