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

Refactor base indicator classes #1446

Merged
merged 10 commits into from
Aug 31, 2023
Merged

Refactor base indicator classes #1446

merged 10 commits into from
Aug 31, 2023

Conversation

aulemahal
Copy link
Collaborator

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)
  • 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?

Move around some of the base indicator classes code. Split the missing values handling from the resampling handling. This way, indicators like return_level can reduce the full time axis and still perform some missing values handling.

Does this PR introduce a breaking change?

No, not yet.

Other information:

@huard @RondeauG, if I am not mistaken, return_level and fit currently have missing values handling disabled because it was impossible with the previous classes to have both this and full reduction of "time". Now that it is possible, should we activate it ?

Similarly, stats had the "Any" missing method forced. Is there a reason for that ? Here, I removed the argument, meaning it will use "from_context".

@aulemahal aulemahal requested review from huard and RondeauG July 31, 2023 18:13
@github-actions github-actions bot added the indicators Climate indices and indicators label Jul 31, 2023
@RondeauG
Copy link
Contributor

if I am not mistaken, return_level and fit currently have missing values handling disabled because it was impossible with the previous classes to have both this and full reduction of "time". Now that it is possible, should we activate it ?

Similarly, stats had the "Any" missing method forced. Is there a reason for that ? Here, I removed the argument, meaning it will use "from_context".

Absolutely ! For all 3, having full control on the missing argument would be a great quality-of-life change.

xclim/core/calendar.py Outdated Show resolved Hide resolved
xclim/core/indicator.py Outdated Show resolved Hide resolved
@Zeitsperre Zeitsperre requested a review from huard August 31, 2023 19:51
@github-actions github-actions bot added the approved Approved for additional tests label Aug 31, 2023
@coveralls
Copy link

Coverage Status

coverage: 90.674%. first build when pulling 71c52a7 on indcls-fullreduce into 36dcbdf on master.

@aulemahal aulemahal merged commit 2490fcc into master Aug 31, 2023
14 checks passed
@aulemahal aulemahal deleted the indcls-fullreduce branch August 31, 2023 21:16
@huard
Copy link
Collaborator

huard commented Sep 18, 2023

Issue CSHS-CWRA/RavenPy#307 is probably due to the fact that this PR made fit a subclass of ReducingIndicator. It was previously a subclass of Indicator, which has no missing value logic in it.

So the generic fit indicator essentially went from skipping missing value checks, to apply the missing algorithm from context (typically any). I think we should have highlighted this more clearly in the release notes.

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 misleadingly characterized as resampling indicator
5 participants