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

Thresholded events - Quantified rate2amount #1778

Merged
merged 26 commits into from
Oct 10, 2024
Merged

Thresholded events - Quantified rate2amount #1778

merged 26 commits into from
Oct 10, 2024

Conversation

coxipi
Copy link
Contributor

@coxipi coxipi commented Jun 13, 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)
  • 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?

  • New thresholded_events generic compute and find_events in run_length. The idea is to find all runs defined by a start and a stop condition and return them along an "event" dimension.
  • It relies on a new function in run_length that can compute the run_length of sequence determined by two conditions. A first condition determines when runs should start, and a second one determines when they stop. This is called runs_with_holes as one possibility with the addtition of the second condition is this: consider a normal run_length, but the second condition allows for holes in thoses sequences, where those holes cannot exceed a given threshold window_stop. runs_with_holes is in fact more general, so perhaps another name should be found.
  • rate2amount and amount2rate can now accept Quantified (str and Quantities), which then requires passing the time coordinate in as the dim argument.

Does this PR introduce a breaking change?

No

Other information:

For the time being, I removed the references to "freezing rain" as I'm not sure I will need the function in xclim. If I do, then, I'll add it back.

@github-actions github-actions bot added the indicators Climate indices and indicators label Jun 13, 2024
@coxipi coxipi marked this pull request as draft June 13, 2024 01:48
xclim/indices/_threshold.py Outdated Show resolved Hide resolved
@coxipi coxipi requested a review from aulemahal June 13, 2024 15:16
xclim/indices/_threshold.py Outdated Show resolved Hide resolved
xclim/indices/generic.py Outdated Show resolved Hide resolved
@coxipi coxipi mentioned this pull request Sep 13, 2024
@aulemahal aulemahal changed the base branch from main to resample-map October 3, 2024 22:24
@aulemahal aulemahal changed the title Freezing rain events Thresholded events - Quantified rate2amount Oct 3, 2024
tests/test_generic.py Outdated Show resolved Hide resolved
@aulemahal
Copy link
Collaborator

@coxipi So I added the ability to find events for each resampling period. I merged in resample-map thinking I could use that stuff here, but as the new functions are multivariate and output a Dataset, it doesn't work...

I edited to the comment with some more info.

xclim/indices/run_length.py Outdated Show resolved Hide resolved
@aulemahal aulemahal marked this pull request as ready for review October 7, 2024 20:54
@aulemahal
Copy link
Collaborator

After discussion with Mrs Verglas herself, I decided that implementing a freezing rain indicator here was not worth the hassle:

  • No standard variable name for freezing rain
  • Very specific and niche
  • The best xclim implementation might not be the optimal performant one, considering that we don't need all outputs of the find_events
  • The potential need to perserve the events would bypass an indicator implemented here anyway

But I did add an application for runs_with_holes by adding yet another parameter to the spell length statistics.

@Zeitsperre Zeitsperre added this to the v0.53.0 milestone Oct 8, 2024
tests/test_run_length.py Outdated Show resolved Hide resolved
@coxipi
Copy link
Contributor Author

coxipi commented Oct 9, 2024

@aulemahal approved (can't approve this PR, I started it)

Copy link
Collaborator

@Zeitsperre Zeitsperre left a comment

Choose a reason for hiding this comment

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

Some minor modifs, but looks good from my perspective

xclim/indicators/atmos/_temperature.py Outdated Show resolved Hide resolved
xclim/indicators/atmos/_temperature.py Outdated Show resolved Hide resolved
xclim/indicators/atmos/_temperature.py Outdated Show resolved Hide resolved
xclim/indices/run_length.py Outdated Show resolved Hide resolved
xclim/indices/run_length.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the approved Approved for additional tests label Oct 9, 2024
aulemahal and others added 3 commits October 9, 2024 16:04
Co-authored-by: Éric Dupuis <71575674+coxipi@users.noreply.github.com>
Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>
Base automatically changed from resample-map to main October 10, 2024 13:37
@aulemahal aulemahal merged commit 96484df into main Oct 10, 2024
21 checks passed
@aulemahal aulemahal deleted the black_ice branch October 10, 2024 14:22
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.

3 participants