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

Choose which dimensions are reduced in measures_improvement #416

Merged
merged 20 commits into from
Nov 4, 2024

Conversation

coxipi
Copy link
Contributor

@coxipi coxipi commented Jun 20, 2024

Pull Request Checklist:

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

  • measures_improvement has an extra kwarg: dim, which allows to control on which dimensions we compute the percentage of improvement.

Does this PR introduce a breaking change?

Default behaviour None is the same as before, reduce all dimensions. I thought it made sense to put dim as the first kwarg, so this is not breaking per-se, but still somewhat semi-breaking for scripts inputting to_level positionnally e.g. measures_improvement([ds1,ds2], to_level), so I can move down dim if it's preferrable.

There is a small breaking change: ds1-ds2 is used instead of ds2 to find non-null values and compute the pct of improvement. This may result in less non-null values. I think it's the right thing to do though.

Other information:

My example is that I had a dataset with (rlat, rlon, period), I want improvement percentages for each period. I realize now this is an unconventional dataset in the xscen philosophy, I should have had separate dataset, when using properties_and_measure I should have used period etc etc. and have multiple catalog entries. But, for properties using group=time.season, time.month, we would have an extra dimension season, month that we don't necessarily want to reduce, so I think it still makes sense.

Also: I think this is unrelated to templates, I assumed this is related to catalogs? So I checked the box

Copy link

github-actions bot commented Jun 20, 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 ✨

@coxipi
Copy link
Contributor Author

coxipi commented Jul 10, 2024

For now, I assume that if dim is precised, it is present in every variable. If we want even more control, we could accept dim as a dict, where every key is a given variable. I think this would not be used that often. With dim=None, all dimensions are reduced like before, regardless if dimensions are different in distinct indicators (e.g. (time) vs. (time,season))

CHANGELOG.rst Outdated Show resolved Hide resolved
src/xscen/diagnostics.py Outdated Show resolved Hide resolved
@coxipi coxipi requested a review from RondeauG October 24, 2024 12:05
Copy link
Collaborator

@RondeauG RondeauG left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll let @juliettelavoie approve the PR since she's a lot more familiar with this part of the code.

src/xscen/diagnostics.py Outdated Show resolved Hide resolved
src/xscen/diagnostics.py Show resolved Hide resolved
tests/test_diagnostics.py Show resolved Hide resolved
@coxipi
Copy link
Contributor Author

coxipi commented Oct 25, 2024

The current test fail is in TestAdjust::test_basic which should not be affected by my changes

Copy link
Contributor

@juliettelavoie juliettelavoie left a comment

Choose a reason for hiding this comment

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

super!
I think the breaking changes for the nan and positional to_level are minor enough.

FYI, The templates are here: https://github.com/Ouranosinc/xscen/tree/main/templates
the first ones uses measures_improvement, but your changes are not breaking enough to break that script.

@RondeauG RondeauG merged commit fbb3c8a into main Nov 4, 2024
16 checks passed
@RondeauG RondeauG deleted the improved_measure_with_dim branch November 4, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants