Allow open bounds in select_time - #2374
Conversation
|
Warning This Pull Request is coming from a fork and must be manually tagged |
Zeitsperre
left a comment
Zeitsperre
left a comment
There was a problem hiding this comment.
I'll let others weigh in on this, but (correct me if I'm wrong) I think the logic tells me the changes may run into problems dealing with 360-day calendars. Is that possible?
Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com> Signed-off-by: Baptiste Hamon <119306054+baptistehamon@users.noreply.github.com>
…nto select-time-bounds
|
You're right, the logic does not work with a 360-day calendar. However, it is not the result of this PR changes as I just realised that the current behaviour of xclim's |
|
@baptistehamon I didn't realize that that functionality wasn't already there. My suggestion would be to open an issue at most. There may be a justification as to why that might be the case, but I'd let my colleague @aulemahal weigh in there as he wrote much of the calendar-handling functionality. Perhaps for here if you want to focus solely on ensuring that the feature supports all existing functionality, that would be enough. Most the team is currently on vacation, but some of them should be back by next week. Thanks again! |
|
I'm open an issue (#2393) regarding the error with 360_days calendar. |
|
I've edited the code to fix small issues and to minimise breaking changes. I added back the old behaviour for closed |
| start = start.expand_dims(time=bnds.time) | ||
| end = end.expand_dims(time=bnds.time) |
There was a problem hiding this comment.
Is there a case where "time" not in start.dims but "time" in end.dims ? Should we handle this too ?
| end = doy_to_days_since(end.convert_calendar(**calkws)) | ||
|
|
||
| # Fill missing values in start and end bounds | ||
| if include_nans or start.isnull().all(): |
There was a problem hiding this comment.
Wouldn't there be a case where start was passed as an array full of NaN and include_nans=False ? I feel like the second part of this condition should test if start was passed a None explicitly.
Also, as far as I understand, I think this is the only compute-triggering line in the function ? Avoiding start.isnull().all() would allow start to be a dask array, no ?
| "Passing open `date_bounds` (i.e., with None) is not supported for 360_day calendars." | ||
| ) | ||
|
|
||
| bnds = time_bnds(da.time.resample(time=bounds_freq if bounds_freq is not None else "YS")) |
There was a problem hiding this comment.
| bnds = time_bnds(da.time.resample(time=bounds_freq if bounds_freq is not None else "YS")) | |
| bnds = time_bnds(da.time.resample(time=bounds_freq or "YS")) |
| ) | ||
|
|
||
| bnds = time_bnds(da.time.resample(time=bounds_freq if bounds_freq is not None else "YS")) | ||
| cal = get_calendar(da) |
There was a problem hiding this comment.
| cal = get_calendar(da) | |
| cal = da.time.dt.calendar |
|
Thanks @baptistehamon ! I made a first pass, nothing major to say here! |
3 participants
Pull Request Checklist:
number) and pull request (:pull:number) has been addedWhat kind of change does this PR introduce?
xclim.core.calendar.mask_between_doystoxclim.core.calendar.select_between_doysdate_boundsanddoy_bounds(i.e.,None) inxclim.core.calendar.select_timeDoes this PR introduce a breaking change?
Yes ! I'll details the breaking changes when I update tests.
Other information: