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

Using map groups in MBCn #1951

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

Using map groups in MBCn #1951

wants to merge 8 commits into from

Conversation

coxipi
Copy link
Contributor

@coxipi coxipi commented Oct 11, 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)
  • 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?

  • MBCn internal functions now use map_groups instead of the groupies-like implementation

Does this PR introduce a breaking change?

No

Other information:

I need to do further tests, but so far it seems as fast, if not faster, than the old implementation. But I have to make tests with bigger datasets.

I tried using the old way, only for training, for a dataset with resolution of 0.11deg over NAM. And the memory was busting, I think there was probably a memory leak in the old way, so that was my motivation to try again to use map_groups, rather than being focused solely on performance.

@github-actions github-actions bot added the sdba Issues concerning the sdba submodule. label Oct 11, 2024
Comment on lines +1870 to +1874
af_q[group.prop] = times.times.values
af_q = af_q.rename({group.prop: "time"})
af_q = af_q.broadcast_like(hist)
# a bit dirty, should avoid this situation before
af_q = af_q.chunk({"time": -1})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a hack: broadcast PROP dimension of af_q to the the full time dimension, so it can be used in map_groups. Perhaps there is already a built-in way to achieve this?

xclim/sdba/_adjustment.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sdba Issues concerning the sdba submodule.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants