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

Check for discharge unit compliance in streamflow statistical indicators #1225

Merged
merged 22 commits into from Dec 19, 2022

Conversation

huard
Copy link
Collaborator

@huard huard commented Nov 8, 2022

Pull Request Checklist:

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

  • Check input data compliance with discharge units in streamflow indicators.
  • Add discharge variable to variables.yml
  • Add test confirming failure and informative error message if units do not comply.
  • Deprecate generic streamflow indicators (fit, stats, freq_analysis)
  • Create generic indicator realm with the indicators fit, stats, and return_level.

Does this PR introduce a breaking change?

Three land indicators for streamflow analysis have been deprecated.

Other information:

@github-actions github-actions bot added docs Improvements to documenation indicators Climate indices and indicators labels Nov 8, 2022
Copy link
Contributor

@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.

Looks good to me!

One remaining issue is that da is very nondescript and doesn't make is obvious that a discharge is required. If we followed conventions established for other functions in xclim, I'd expect q or something similar. However, I understand that changing it at this point would likely be a breaking change...

@github-actions github-actions bot added the approved Approved for additional tests label Nov 8, 2022
@github-actions github-actions bot removed the docs Improvements to documenation label Nov 8, 2022
@huard
Copy link
Collaborator Author

huard commented Nov 8, 2022

It's because the stats indices are generic, and are meant to be applicable to any kind of variable.

Unless @aulemahal has magic trick up his sleeve ?

@huard
Copy link
Collaborator Author

huard commented Nov 14, 2022

This PR took an unexpected turn. It now includes a new generic realm to hold stats, fit and return_level (previously freq_analysis.

@RondeauG
Copy link
Contributor

I'll let the others approve the more internal changes, but it looks good to me.

@coveralls
Copy link

coveralls commented Dec 9, 2022

Coverage Status

Coverage increased (+0.03%) to 92.022% when pulling 542708f on fix-1130 into 503c506 on master.

@aulemahal aulemahal merged commit b6f6c08 into master Dec 19, 2022
@aulemahal aulemahal deleted the fix-1130 branch December 19, 2022 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests indicators Climate indices and indicators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

frequency_analysis indicator weirdly declared
5 participants