Correctly interpolate seasons in Grouper

[saschahofmann]

Pull Request Checklist:

• This PR addresses an already opened issue (for bug fixes / features)
  • This PR fixes only part of Linear interpolation for seasonal bias adjustment leads to non-smooth distribution #2014
• Tests for the changes have been added (for bug fixes / features)
• CHANGELOG.rst has been updated (with summary of main changes)
  • Link to issue (:issue:number) and pull request (:pull:number) has been added

What kind of change does this PR introduce?

This PR adds a line to correctly interpolate seasonal values. It also changes the test_timeseries function that now accepts a calendar argument instead of cftime. Not providing it or providing None is equivalent to cftime=False and calendar='standard to the previous cftime=True. This allows for testing different calendar implementations e.g. 360_day calendars

[saschahofmann]

I just realised that the factor of 1/6 is assuming that all seasons have the same length which in gregorian calendars is not necessarily true but I am not sure it matters too much at least the function should be smooth.

[saschahofmann]

Just to prove that this leads to a smooth result, same input as in the issue:

[github-actions]

Warning

This Pull Request is coming from a fork and must be manually tagged approved in order to perform additional testing.

[saschahofmann]

Weirdly and contrary to what I showed yesterday, today I am still getting clear transitions as if there still wasn't any linear interpolation.

[Zeitsperre]

@saschahofmann We recently changed the layout of xclim to use a src structure. It might be worthwhile to try reinstalling the library.

[saschahofmann]

I reinstalled xclim but I am still getting very similar results to before the "fix". You have any advice on where else I could look?

[Zeitsperre]

I reinstalled xclim but I am still getting very similar results to before the "fix". You have any advice on where else I could look?

Could it be that you have obsolete __pycache__ folders still among your cloned folders? @coxipi is looking into recreating your example based on your branch for validation, but if the tests are working as intended on CI, then it's likely a caching/installation issue.

[coxipi]

I managed to install the environment, for some reason I only had the branch "main" when I cloned the fork yesterday

• I confirmed that the function has the appropriate modifications inside the notebook I'm using

import inspect
print(inspect.getsource(sdba.base.Grouper.get_index))

• I also find that the interpolation is wrong.

I'll try to have look later. Maybe the interp boolean condition is not triggered properly?

[saschahofmann]

I am pretty sure that the get_index function is updated in my notebook. Either I am wrong in expecting a smoother result (it seems to have changed slightly to what I got earlier) or there is something else going on. I will keep investigating

[coxipi]

It's simply interp which can't be "nearest", otherwise no interpolation takes place ... I think our only other option is linear.

from xclim import sdba
QM = sdba.EmpiricalQuantileMapping.train(
    ref, hist, nquantiles=15, group="time.season", kind="+"
)

scen = QM.adjust(sim, extrapolation="constant", interp="nearest")
scen_interp = QM.adjust(sim, extrapolation="constant", interp="linear")
outd = {
    "Reference":ref,
    "Model - biased":hist,
    "Model - adjusted - no interp":scen, 
    "Model - adjusted - linear interp":scen_interp, 
}
for k,da in outd.items(): 
    da.groupby("time.dayofyear").mean().plot(label=k)
plt.legend()

This doesn't reproduce your figure however. It seems your figure above was matching the reference very well, better than what I have even with the linear interpolation. But it does get rid of obvious discontinuities.

[coxipi]

There is clearly something wrong going on. Comparing
hist - scen_month
scen_time - scen_month
scen_season - scen_month
scen_month - scen_month

scen_season is way off

[saschahofmann]

@coxipi I think only mention this in the original issue: my analysis is done with QuantileDeltaMapping instead of EmpiricalQuantileMapping. Here the equivalent chart to yours for that:

season still seem kinda weird

[saschahofmann]

A similar trend becomes apparent when looking at the adjusted - historical (now for EmpiricalQuantileMapping)

[coxipi]

Yes, I have seen simlilar things by playing with the choice of how get_index. I feel this should not be this sensitive. Let me try and get this back

[saschahofmann]

And another fix for using sdba.Scaling in utils.pyL222

[saschahofmann]

Merged @aulemahal fix and this is now ready for merging

[aulemahal]

Looks good! Thanks!

I think we can avoid a breaking change in test_timeseries.

[saschahofmann]

From my side this looks good not sure how to trigger the coverage pipeline?

[saschahofmann]

@Zeitsperre I added these two lines to allow linear interpolation with dayofyear but I am wondering whether this combination makes sense?
https://github.com/Ouranosinc/xclim/pull/2019/files#diff-3fdadf9776d54afe05de332ab8810fc3d2939d75bd98273f3661f05d71f296a4R314-R315

It was failing in a test

[Zeitsperre]

I added these two lines to allow linear interpolation with dayofyear but I am wondering whether this combination makes sense? https://github.com/Ouranosinc/xclim/pull/2019/files#diff-3fdadf9776d54afe05de332ab8810fc3d2939d75bd98273f3661f05d71f296a4R314-R315

It was failing in a test

I'm going to defer to @coxipi or @aulemahal here. This is a bit out of my depth haha

[coxipi]

@Zeitsperre I added these two lines to allow linear interpolation with dayofyear but I am wondering whether this combination makes sense? https://github.com/Ouranosinc/xclim/pull/2019/files#diff-3fdadf9776d54afe05de332ab8810fc3d2939d75bd98273f3661f05d71f296a4R314-R315

It was failing in a test

I don't think there would be a point in doing linear interpolation for dayofyear. We have adjusting factors for each dayofyear, so we don't have in-between values where we need to interpolate the training data. I would say we probably need to change the failing test, let me see

I don't see the failing, can you point to a specific commit?

[saschahofmann]

Yes. Its this test TestExtremeValues.test_real_data: https://github.com/Ouranosinc/xclim/actions/runs/12985440090/job/36210393696

I fixed it by allowing again linear interp of dayofyear. I think it was allowed previously but I slightly restructured the code so now by default it was disabled.

[saschahofmann]

I reverted the change that allows linear interp with dayofyear grouping and instead changed the test to use nearest

[saschahofmann]

@coxipi I think maybe reenabling linear interpolation from dayofyear for now is a better solution. Since otherwise it represents a breaking change, e.g. right now the tutorial does some similar (hence the breaking test in sdba-advanced.ipynb) where we do EQM with dayofyer with linear interp. Instead I could add a deprecation warning for maybe 0.57.0?

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[coveralls]

coverage: 89.94% (-0.03%) from 89.974%
when pulling cf45fd8 on saschahofmann:fix-#2014
into 3152070 on Ouranosinc:main.