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

Add snow water cover indices #1275

Merged
merged 38 commits into from Feb 28, 2023
Merged

Add snow water cover indices #1275

merged 38 commits into from Feb 28, 2023

Conversation

coxipi
Copy link
Contributor

@coxipi coxipi commented Jan 18, 2023

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

  • New indices (snow cover duration/start/end) for snow amount

Does this PR introduce a breaking change?

Other information:

@github-actions github-actions bot added the indicators Climate indices and indicators label Jan 18, 2023
HISTORY.rst Outdated Show resolved Hide resolved
HISTORY.rst Outdated Show resolved Hide resolved
xclim/indicators/land/_snow.py Outdated Show resolved Hide resolved
xclim/indicators/land/_snow.py Outdated Show resolved Hide resolved
xclim/indicators/land/_snow.py Outdated Show resolved Hide resolved
@Zeitsperre
Copy link
Collaborator

Ignore the license failure. Already resolved.

@coxipi coxipi marked this pull request as ready for review February 6, 2023 19:00
Copy link
Collaborator

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

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

Few comments on the metadata. The code looks good.

I think we need a clearer syntax for constructing indicators metadata.

When we get to a point where most basic indicators like these can be constructed from generic functions, having explicit names (rather than poetic) will be a plus. Thus, I am wondering if we could rename these like:

continuous_snow_amount_cover_start -> snw_season_start
snow_depth_cover_duration -> snd_season_length
etc

Where season has this "long spell with small holes" meaning, like in the rest of xclim.

This is a suggestion, we might need to discuss it in the meeting tomorrow.

xclim/data/fr.json Outdated Show resolved Hide resolved
xclim/data/fr.json Outdated Show resolved Hide resolved
xclim/data/fr.json Outdated Show resolved Hide resolved
xclim/data/fr.json Outdated Show resolved Hide resolved
xclim/data/fr.json Outdated Show resolved Hide resolved
xclim/data/fr.json Outdated Show resolved Hide resolved
xclim/data/fr.json Outdated Show resolved Hide resolved
xclim/data/fr.json Outdated Show resolved Hide resolved
xclim/indicators/land/_snow.py Outdated Show resolved Hide resolved
xclim/indicators/land/_snow.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the CI Automation and Contiunous Integration label Feb 21, 2023
@github-actions github-actions bot removed the CI Automation and Contiunous Integration label Feb 21, 2023
Ludwig.Lierhammer and others added 2 commits February 22, 2023 09:20
@github-actions github-actions bot added the docs Improvements to documenation label Feb 22, 2023
HISTORY.rst Outdated Show resolved Hide resolved
@coxipi
Copy link
Contributor Author

coxipi commented Feb 27, 2023

Should I leave snow_depth_from_amount or change this to snd_from_snw? Or even snw_to_snd @aulemahal @Zeitsperre

HISTORY.rst Outdated Show resolved Hide resolved
xclim/indicators/land/_snow.py Outdated Show resolved Hide resolved
xclim/indicators/land/_snow.py Outdated Show resolved Hide resolved
xclim/indices/_threshold.py Outdated Show resolved Hide resolved
xclim/indices/_threshold.py Outdated Show resolved Hide resolved
xclim/indices/_threshold.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the approved Approved for additional tests label Feb 27, 2023
coxipi and others added 6 commits February 27, 2023 15:46
Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>
Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>
english 2/2

Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>
@Zeitsperre Zeitsperre merged commit 27429d6 into master Feb 28, 2023
@Zeitsperre Zeitsperre deleted the add_snow_water_cover_indices branch February 28, 2023 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests docs Improvements to documenation indicators Climate indices and indicators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants