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

reorder with window #1566

Merged
merged 13 commits into from
Feb 19, 2024
Merged

reorder with window #1566

merged 13 commits into from
Feb 19, 2024

Conversation

juliettelavoie
Copy link
Contributor

@juliettelavoie juliettelavoie commented Dec 18, 2023

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?

  • Allow reorder on group including a window

Does this PR introduce a breaking change?

Previous behavior was to ignored the window (silently) and only do the reorder on the group (eg. dayofyear), now the window is considered.

Other information:

This is for MBCn. In an email, Cannon said: "All parts of the process, including the reordering, are done on your subset, including the buffer around the day of interest. If you’re using 31 days x 30 years then you have 930 data points. Bias adjustment would be performed on these 930 cases. At the end, you’d reorder according to the QDM ranks and select the middle day. Repeat for the remaining days of the year."

@github-actions github-actions bot added the sdba Issues concerning the sdba submodule. label Dec 18, 2023
xclim/sdba/_processing.py Outdated Show resolved Hide resolved
xclim/sdba/_processing.py Outdated Show resolved Hide resolved
xclim/sdba/_processing.py Outdated Show resolved Hide resolved
xclim/sdba/_processing.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

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

This fell between the cracks, sorry. AFAIU, this looks good to go. Except for a nitpicky detail in the conditional.

xclim/sdba/_processing.py Outdated Show resolved Hide resolved
@Zeitsperre Zeitsperre mentioned this pull request Feb 19, 2024
5 tasks
@Zeitsperre Zeitsperre added the approved Approved for additional tests label Feb 19, 2024
else:
raise ValueError(
f"Reordering can only be done along one dimension."
f" If there is more than one, one of the dimension should be `window`."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
f" If there is more than one, one of the dimension should be `window`."
f" If there is more than one, they should be `window` and `time`."

@aulemahal aulemahal merged commit 1508be0 into master Feb 19, 2024
19 checks passed
@aulemahal aulemahal deleted the reorder branch February 19, 2024 17:14
Zeitsperre added a commit that referenced this pull request Feb 19, 2024
### What kind of change does this PR introduce?

* Prepares the next release (v0.48.0)

### Does this PR introduce a breaking change?

No.

### Other information:

This is waiting on pull requests #1566 and #1643
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests sdba Issues concerning the sdba submodule.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants