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.

Sign up for GitHub

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

Jump to bottom

Frequency analysis misleadingly characterized as resampling indicator #1263

Closed
aulemahal opened this issue Dec 22, 2022 · 0 comments · Fixed by #1446
Closed

Frequency analysis misleadingly characterized as resampling indicator #1263

aulemahal opened this issue Dec 22, 2022 · 0 comments · Fixed by #1446
Assignees
@huard @aulemahal
Labels
invalid This doesn't seem right
Milestone
v0.45.0

Comments

@aulemahal
Copy link
Collaborator

aulemahal commented Dec 22, 2022 •
edited
Loading

xclim/xclim/indicators/land/_streamflow.py

Line 73 in c8f45a7

freq_analysis = FA(

where FA is a subclass of ResamplingIndicator. However, the index reduces the time axis completely, which is not what we would expect from a ResamplingIndicator that exposes a freq argument.

Maybe we should have a new subclass for this type of indicator and rename the freq argument to make it less misleading.
@Zeitsperre Zeitsperre added the invalid This doesn't seem right label Jun 13, 2023
@Zeitsperre Zeitsperre added this to the Summer 2023 milestone Jun 13, 2023
@huard huard self-assigned this Jun 13, 2023
@Zeitsperre Zeitsperre modified the milestones: Summer 2023, v0.45.0 Jul 25, 2023
@aulemahal aulemahal self-assigned this Jul 27, 2023
@aulemahal aulemahal mentioned this issue Jul 31, 2023
Merged
5 tasks
aulemahal added a commit that referenced this issue Aug 31, 2023
@aulemahal
Refactor base indicator classes (#1446)
2490fcc 
<!--Please ensure the PR fulfills the following requirements! -->
<!-- If this is your first PR, make sure to add your details to the
AUTHORS.rst! -->
### Pull Request Checklist:
- [x] This PR addresses an already opened issue (for bug fixes /
features)
    - This PR fixes #1263
- [ ] Tests for the changes have been added (for bug fixes / features)
- [x] (If applicable) Documentation has been added / updated (for bug
fixes / features)
- [x] CHANGES.rst has been updated (with summary of main changes)
- [x] 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".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@huard huard

@aulemahal aulemahal

Labels
invalid This doesn't seem right
Projects
None yet
Milestone
v0.45.0
Development

Successfully merging a pull request may close this issue.

Refactor base indicator classes Ouranosinc/xclim
3 participants
@huard @Zeitsperre @aulemahal