-
-
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
Add SeasonGrouper, SeasonResampler #9524
base: main
Are you sure you want to change the base?
Conversation
54b2ef1
to
594d4a7
Compare
First comment, but I have performed only quick test 1 -In your short example, it's probably : Or my Github knowledge is too limited, and I'm not testing the right branch. 2 - Season grouperSeems OK for all I have tested. In particular I can :
3 - Season resamplerWorks as expected from the example. It could be useful to have a NaN value for an incomplete season : the first DJF cannot not be computed, and is not. This mean that the first value is not a DJF one, but a MAM value. Could be a bit misleading. 4 - cftimeI have tested it with cftime calendars instead of datetime. It works with the traditional calendar (gregorian, standard). But not with others like 360_day, 365_day, julian., proleptic_gregorian : 5 - Simple dataI've build a dataset with the number ot the month as a variable. So I'm sure that the computation is correct. Thanks' for these features. They are quit easy and straigthforward to use. In particular, it allows to work on variables, as xcdat features work on Dataset only, which yields a more complicated syntax. I'm gonna try to imagine further tests. Olivier |
Thanks @oliviermarti ! this is incredibly helpful
Yes, my mistake. I fixed the snippet.
This should not work, did you really get correct results.
The |
In fact not ! Only the first value is correct. A bit dangerous that it returns a result and not an error.
Olivier |
Hi @dcherian, thank you for this PR! I've been looking forward to having this feature in Xarray. No guarantees on a timeline, but I plan to start looking at this PR this week. I'll experiment with this feature and see how I can leverage it to simplify xCDAT PR #423 for custom seasons. I'll also try to contribute any useful tests. |
These two groupers allow defining custom seasons, and dropping incomplete seasons from the output. Both cases are treated by adjusting the factorization -- conversion from group labels to integer codes -- appropriately.
9180536
to
77dc5e0
Compare
Hey @dcherian, quick question. Will this PR add support for using For example, if I wanted to perform grouped averaging on year and custom seasons it might look like: ds.air.groupby(time=[ds.time.dt.year, SeasonGrouper(["JF", "MAM", "JJAS", "OND"])]).mean() |
Another question: If we're defining custom seasons with months that span the calendar year, those months are from the previous year correct? For example for "NDJFM", "ND" should be from the previous year. air.groupby(year=UniqueGrouper(), time=SeasonGrouper(["NDJFM"])) |
Yes it tried to be that smart |
@tomvothecoder @oliviermarti i fixed the existing tests now, please try it out! FWIW the need to support |
7aaafb2
to
c66ad96
Compare
I'm writing a few tests right now. How do you want me to add them to your fork branch?
I noticed in a test I'm writing for the above code that "ND" is being taken from the same year, not the previous year. I think we expect the previous year "ND" to be used instead. I will show a clear example once I add the test. |
Ah nice find. A PR to this branch should be the easiest |
These two groupers allow defining custom seasons, and dropping incomplete seasons from the output. Both cases are treated by adjusting the factorization -- conversion from group labels to integer codes -- appropriately.
The last piece from #8509
whats-new.rst
api.rst
Example:
TODO:
drop_incomplete
in SeasonGroupercc @tomvothecoder do you have time to contribute some tests? I bet we'll simplify a bunch of xcdat this way, and you probably already have tests :)