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

Snow season and days above #1708

Merged
merged 8 commits into from
Apr 18, 2024
Merged

Snow season and days above #1708

merged 8 commits into from
Apr 18, 2024

Conversation

aulemahal
Copy link
Collaborator

@aulemahal aulemahal commented Apr 16, 2024

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?

  • Renames old sn[dw]_season_length to sn[dw]_days_above, which better reflects what they actually do.
  • Implement sn[dw]_season_length as an actual season length : duraction between a start and an end, both defined as the first day of a period of minimum length continuously above/under a given threshold.
  • Rephrase the documentation of these indicators and of the start/end variants.
  • Update the threshold of snw, like we said we'd do in 0.47 (youpsi).

Does this PR introduce a breaking change?

Yes. An indicator name now points to another computation.

Other information:

@github-actions github-actions bot added the indicators Climate indices and indicators label Apr 16, 2024
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.

LGTM. I wish there was an easier way to have a transitional set of functions, but there isn't really. Your approach makes sense.

xclim/indices/_threshold.py Show resolved Hide resolved
xclim/indices/_threshold.py Show resolved Hide resolved
xclim/indicators/land/_snow.py Show resolved Hide resolved
)

snw_days_above = SnowWithIndexing(
title="Days with snow (amount)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Days with snow greater than amount"?

CHANGES.rst Show resolved Hide resolved
@github-actions github-actions bot added the approved Approved for additional tests label Apr 17, 2024
Copy link
Contributor

@coxipi coxipi left a comment

Choose a reason for hiding this comment

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

Approved!

xclim/indices/_threshold.py Outdated Show resolved Hide resolved
xclim/data/fr.json Outdated Show resolved Hide resolved
@aulemahal
Copy link
Collaborator Author

@Zeitsperre it seems the doctests do not see the testing data ? Do you know what is happening ?

@Zeitsperre
Copy link
Collaborator

@aulemahal I think the doctests are flakier than we imagined. It looks like there are data races / unsafe asynchronous read events that are causing problems. Re-running the failed tests might help for here, but we should address this ASAP. Will open an issue.

@coveralls
Copy link

coveralls commented Apr 18, 2024

Coverage Status

coverage: 90.65% (-0.05%) from 90.703%
when pulling 130cf67 on upd-snow-season
into d27b4ed on main.

@aulemahal
Copy link
Collaborator Author

The py310-coverage-lmoments-doctest test was cancelled again, after 10m 3s, is this a timeout issue ? It was almost done...

@aulemahal aulemahal merged commit a40069b into main Apr 18, 2024
19 checks passed
@aulemahal aulemahal deleted the upd-snow-season branch April 18, 2024 17:50
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.

Snow season length is not a season length
5 participants