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

dOTC #1787

Merged
merged 106 commits into from
Aug 7, 2024
Merged

dOTC #1787

merged 106 commits into from
Aug 7, 2024

Conversation

LamAdr
Copy link
Collaborator

@LamAdr LamAdr commented Jun 18, 2024

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • Tests for the changes have been added (for bug fixes / features)
    • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • CHANGES.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?

  • Introduce dOTC bias correction method into sdba.

Does this PR introduce a breaking change?

No

Other information:

Am I doing this right?

Copy link

github-actions bot commented Jun 18, 2024

Welcome, new contributor!

It appears that this is your first Pull Request. To give credit where it's due, we ask that you add your information to the AUTHORS.rst and .zenodo.json:

  • The relevant author information has been added to AUTHORS.rst and .zenodo.json

Please make sure you've read our contributing guide. We look forward to reviewing your Pull Request shortly ✨

@github-actions github-actions bot added the sdba Issues concerning the sdba submodule. label Jun 18, 2024
@Zeitsperre
Copy link
Collaborator

Am I doing this right?

You are!

Once you address the errors brought up by pre-commit, the tests should run. From what I can tell, there are a few programming errors and some style problems. To make things easier, try installing pre-commit beforehand to have the checks run when committing:

$ pip (or conda) install pre-commit
$ pre-commit install

@LamAdr
Copy link
Collaborator Author

LamAdr commented Jun 18, 2024

NB : I added "ot" to codespell's ignore list

xclim/sdba/adjustment.py Outdated Show resolved Hide resolved
xclim/sdba/adjustment.py Outdated Show resolved Hide resolved
xclim/sdba/_adjustment.py Outdated Show resolved Hide resolved
@coxipi
Copy link
Contributor

coxipi commented Jun 18, 2024

Very nice! The code is very readable, well organized, good job!

In one of my comment where I mention a possible function _otc_adjust, you can also see I mention map_blocks. Basically, your are currently doing the correction (for OTC):

` tasmax_1950-1980, pr_1950-1980 -> [pr_1950-1980]', [pr_1950-1980]' ` 

where the full 30 years of climatology are used in training/adjusting.

It is often useful to have different time grouping, e.g. put all January 1rst together and correct them apart, put all Jan. 2nd together, correct them separately, etc.

` tasmax_01-01-19XY, pr_01-01-19XY -> [pr_01-01-19XY]', [pr_01-01-19XY]' ` 
` tasmax_02-01-19XY, pr_02-01-19XY -> [pr_02-01-19XY]', [pr_02-01-19XY]' ` 

If you look at class EmpiricalQuantileMapping, you will see that eqm_train can receive the argument "group". To reproduce the case you were doing, you set:

group = "time"

to have the second example I'm describing, you would set:

group = "time.dayofyear"

this tells to the code, group data in days of year, i.e. Jan.1, Jan.2, Jan.3, etc. This would be nice to have.

xclim/sdba/utils.py Outdated Show resolved Hide resolved
xclim/sdba/adjustment.py Outdated Show resolved Hide resolved
Copy link
Contributor

@SarahG-579462 SarahG-579462 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

docs/notebooks/sdba.ipynb Outdated Show resolved Hide resolved
xclim/sdba/_adjustment.py Show resolved Hide resolved
xclim/sdba/adjustment.py Show resolved Hide resolved
.zenodo.json Outdated Show resolved Hide resolved
xclim/sdba/_adjustment.py Outdated Show resolved Hide resolved
xclim/sdba/_adjustment.py Outdated Show resolved Hide resolved
xclim/sdba/_adjustment.py Show resolved Hide resolved
xclim/sdba/_adjustment.py Show resolved Hide resolved
xclim/sdba/_adjustment.py Show resolved Hide resolved
xclim/sdba/adjustment.py Show resolved Hide resolved
@LamAdr
Copy link
Collaborator Author

LamAdr commented Aug 7, 2024

@Zeitsperre
I guess the macos fails because SBCK isn't installed.
Do you understand why Python3.9 (ubuntu-latest, conda) fails?

@Zeitsperre
Copy link
Collaborator

@LamAdr No worries, there's some behaviour change in the latest version of dask and it's impacting both conda and macOS builds right now. The fix is already in #1870 and will be merged soon.

@Zeitsperre
Copy link
Collaborator

@Zeitsperre I guess the macos fails because SBCK isn't installed.

Indeed. You need to add another pytest.importorskip on the affected tests. SBCK is only installed in two builds on CI.

@Zeitsperre
Copy link
Collaborator

@LamAdr Is this pull request ready? If so, it'll be the last one before we tag a new release (ideally, tomorrow).

Let me know if you want more time to do any finishing touches!

@LamAdr
Copy link
Collaborator Author

LamAdr commented Aug 7, 2024

@Zeitsperre Yep, ready.
Thanks again

@LamAdr LamAdr merged commit 721375f into Ouranosinc:main Aug 7, 2024
19 checks passed
@LamAdr LamAdr deleted the dOTC branch August 7, 2024 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests CI Automation and Contiunous Integration docs Improvements to documenation sdba Issues concerning the sdba submodule.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants