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

Clarifying instructions in "First example : pr and frequency adaptation" example #1740

Closed
1 task done
bweeding opened this issue May 2, 2024 · 2 comments · Fixed by #1742
Closed
1 task done

Clarifying instructions in "First example : pr and frequency adaptation" example #1740

bweeding opened this issue May 2, 2024 · 2 comments · Fixed by #1742
Assignees
Labels
invalid This doesn't seem right priority Immediate priority support Questions and help for users/developers

Comments

@bweeding
Copy link

bweeding commented May 2, 2024

Generic Issue

Posting as I'm confused about the example code provided for adjusting precipitation data including frequency adjustment.

In the code below from https://xclim.readthedocs.io/en/stable/notebooks/sdba.html#First-example-:-pr-and-frequency-adaptation, the training appears to take place without reference to pr_hist:

# 2nd try with adapt_freq
sim_ad, pth, dP0 = sdba.processing.adapt_freq(
    pr_ref, pr_sim, thresh="0.05 mm d-1", group="time"
)
QM_ad = sdba.EmpiricalQuantileMapping.train(
    pr_ref, sim_ad, nquantiles=15, kind="*", group="time"
)
scen_ad = QM_ad.adjust(pr_sim)

Is that correct? Or should pr_hist be involved in generating QM_ad, which is then applied to sim_ad?

Cheers
Ben

Code of Conduct

  • I agree to follow this project's Code of Conduct
@aulemahal
Copy link
Collaborator

Oh, hey you are right, this is an error! Indeed, pr_hist should be the second input of adapt_freq and hist_ad, its output, be the input of the train step.

However, pr_sim is correctly the input of the adjust step here. The primary goal of adapt_freq is to avoid problems in the adjustment factors computed by the train. Once we have the bug-free adjustment factors, there's no reason to use the inflated version of the precipitation to create scen.

At least, that's what I understand from the reference article. Their adjustment method is not exactly xclim's EQM, but the idea is the same.
Themeßl et al. - 2012 - Empirical-statistical downscaling and error correc.pdf

@Zeitsperre Zeitsperre added invalid This doesn't seem right support Questions and help for users/developers labels May 2, 2024
@Zeitsperre Zeitsperre added this to the v0.49.0 (PyCon LT) milestone May 2, 2024
@Zeitsperre Zeitsperre added the priority Immediate priority label May 2, 2024
@bweeding
Copy link
Author

bweeding commented May 2, 2024

Ah good, that's what I've done in my code:

hist_fad, pth, dP0 = sdba.processing.adapt_freq(
    agcd.pr.chunk(chunks={"time":10950,"lon": 25, "lat": 25}), cnrm_hist.pr.chunk(chunks={"time":10950,"lon": 25, "lat": 25}), thresh="0.05 mm d-1", group="time"
)

proj_fad, pth, dP0 = sdba.processing.adapt_freq(
    agcd.pr.chunk(chunks={"time":10950,"lon": 25, "lat": 25}), cnrm_proj85.pr.chunk(chunks={"time":34310,"lon": 25, "lat": 25}), thresh="0.05 mm d-1", group="time"
)

cur_trainer = sdba.QuantileDeltaMapping.train(
    agcd.pr.chunk(chunks={"time":10950,"lon": 25, "lat": 25}), hist_fad.chunk(chunks={"time":10950,"lon": 25, "lat": 25}), nquantiles=15, kind="*", group="time"
)

hist_adj_out = cur_trainer.adjust(cnrm_hist.pr.chunk(chunks={"time":10950,"lon": 25, "lat": 25}), interp="linear")

proj_adj_out = cur_trainer.adjust(cnrm_proj85.pr.chunk(chunks={"time":34310,"lon": 25, "lat": 25}), interp="linear")

aulemahal added a commit that referenced this issue May 2, 2024
<!--Please ensure the PR fulfills the following requirements! -->
<!-- If this is your first PR, make sure to add your details to the
AUTHORS.rst! -->
### Pull Request Checklist:
- [x] This PR addresses an already opened issue (for bug fixes /
features)
    - This PR fixes #1740
- [ ] Tests for the changes have been added (for bug fixes / features)
- [x] (If applicable) Documentation has been added / updated (for bug
fixes / features)
- [x] CHANGES.rst has been updated (with summary of main changes)
- [x] Link to issue (:issue:`number`) and pull request (:pull:`number`)
has been added

### What kind of change does this PR introduce?
In the frequency adaptation example, `pr_sim` was sent to `adapt_freq`
and then to the `train`. However, this should be `hist` instead.

### Does this PR introduce a breaking change?
No.

### Other information:
Also changed the year in the citation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right priority Immediate priority support Questions and help for users/developers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants