Skip to content
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

Correctly interpolate seasons in Grouper #2019

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

saschahofmann
Copy link
Contributor

Pull Request Checklist:

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

@github-actions github-actions bot added the sdba Issues concerning the sdba submodule. label Dec 11, 2024
@saschahofmann
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@Zeitsperre Zeitsperre requested a review from aulemahal December 11, 2024 16:53
@Zeitsperre Zeitsperre added bug Something isn't working standards / conventions Suggestions on ways forward labels Dec 11, 2024
Copy link

Warning

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

@saschahofmann
Copy link
Contributor Author

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
Copy link
Collaborator

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

@Zeitsperre Zeitsperre mentioned this pull request Dec 12, 2024
5 tasks
@github-actions github-actions bot added the docs Improvements to documenation label Dec 12, 2024
@saschahofmann
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor

coxipi commented Dec 13, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

coxipi commented Dec 16, 2024

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()

image

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
Copy link
Contributor

coxipi commented Dec 16, 2024

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

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working docs Improvements to documenation sdba Issues concerning the sdba submodule. standards / conventions Suggestions on ways forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linear interpolation for seasonal bias adjustment leads to non-smooth distribution
3 participants