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

Add Christmas Day indices, add bivariate_count_occurrences #2030

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Add Christmas Day indices, add bivariate_count_occurrences #2030

wants to merge 17 commits into from
+448 −40

Conversation

Zeitsperre
Copy link
Collaborator

@Zeitsperre Zeitsperre commented Dec 19, 2024 •
edited
Loading

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

  • Adds two new indices and indicators:
    • holiday_snow_days
    • holiday_snow_and_snowfall_days

Does this PR introduce a breaking change?

No.

Other information:

These indices are currently designed to return a boolean array, i.e. if December 25th (or the range of days asked for) satisfy the requirements, the time step at the frequency provided is marked True.

There's also the idea of using the days_with_snow indice by passing a date-constrained snow depth array to it. I'm not sure which approach is ideal.

@Zeitsperre Zeitsperre added this to the v0.55.0 milestone Dec 19, 2024
@Zeitsperre Zeitsperre requested a review from coxipi December 19, 2024 22:53
@Zeitsperre Zeitsperre self-assigned this Dec 19, 2024
@github-actions github-actions bot added the indicators Climate indices and indicators label Dec 19, 2024
@github-actions github-actions bot added the CI Automation and Contiunous Integration label Dec 20, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 20, 2024 •
edited
Loading

Note

It appears that this Pull Request modifies the main.yml workflow.

On inspection, the XCLIM_TESTDATA_BRANCH environment variable is set to the most recent tag (v2025.1.8).

No further action is required.

@Zeitsperre
add holiday snow indicator tests
03d24ca 
Signed-off-by: Trevor James Smith <[email protected]>
@Zeitsperre
update CHANGELOG.rst
fc691a2 
Signed-off-by: Trevor James Smith <[email protected]>
@Zeitsperre
Copy link
Collaborator Author

This is just waiting on Ouranosinc/xclim-testdata#30

@Zeitsperre Zeitsperre requested a review from aulemahal December 20, 2024 20:58
aulemahal
aulemahal requested changes Jan 8, 2025
View reviewed changes
src/xclim/indices/_threshold.py Outdated Show resolved Hide resolved
src/xclim/indices/_threshold.py Outdated Show resolved Hide resolved
src/xclim/indicators/land/_snow.py Outdated Show resolved Hide resolved
src/xclim/indicators/land/_snow.py Outdated Show resolved Hide resolved
Zeitsperre and others added 3 commits January 8, 2025 12:38
@Zeitsperre Zeitsperre requested a review from aulemahal January 8, 2025 17:43
@Zeitsperre
update testdata pin
0050cfe 
Signed-off-by: Trevor James Smith <[email protected]>
@Zeitsperre
Copy link
Collaborator Author

One thing I'm wondering about are the names of these indices/indicators. Given that they can accept an indexer, would it make more sense to general the indices more and rename the indicators to use "Christmas" as well as hardcode them to December 25th?

@tlogan2000 any opinions on this?

@aulemahal
Copy link
Collaborator

If you hardcode the date to a specific date, then there's no need for a compute function, you could simple use generic.count_occurrences. And I would argue that the more general version of holidays_snow_days is snd_days_above.

I think generalizing holidays_snow_and_snowfall_days would best be done by implementing a count_multivariate_occurrences.

@Zeitsperre Zeitsperre changed the title Add Christmas Day indices Add Christmas Day indices, add bivariate_count_occurrences Jan 9, 2025
aulemahal
aulemahal reviewed Jan 9, 2025
View reviewed changes
src/xclim/data/fr.json Outdated Show resolved Hide resolved
src/xclim/indices/_threshold.py Outdated Show resolved Hide resolved
src/xclim/indices/_threshold.py Outdated Show resolved Hide resolved
src/xclim/indices/_threshold.py Outdated Show resolved Hide resolved
src/xclim/indices/_threshold.py Outdated Show resolved Hide resolved
src/xclim/indices/_threshold.py Outdated Show resolved Hide resolved
src/xclim/indices/_threshold.py Outdated Show resolved Hide resolved
src/xclim/indices/_threshold.py Outdated Show resolved Hide resolved
@Zeitsperre
synchronize some dependencies
a18d662 
Signed-off-by: Trevor James Smith <[email protected]>
@Zeitsperre
add workaround for pygments breaking changes with sphinx-codeautolink…
876a1fb 
…, update nbconvert

Signed-off-by: Trevor James Smith <[email protected]>
@Zeitsperre Zeitsperre requested a review from aulemahal January 13, 2025 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coxipi coxipi Awaiting requested review from coxipi

@aulemahal aulemahal Awaiting requested review from aulemahal

Requested changes must be addressed to merge this pull request.

Assignees

@Zeitsperre Zeitsperre

Labels
CI Automation and Contiunous Integration indicators Climate indices and indicators
Projects
None yet
Milestone
v0.55.0
Development

Successfully merging this pull request may close these issues.

Add White Christmas and Perfect Christmas indicators
2 participants
@Zeitsperre @aulemahal