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

Number of days can be both nan and 0 #1491

Closed
1 task done
KatharinaBuelow opened this issue Oct 5, 2023 · 1 comment · Fixed by #1492
Closed
1 task done

Number of days can be both nan and 0 #1491

KatharinaBuelow opened this issue Oct 5, 2023 · 1 comment · Fixed by #1492
Milestone
v0.46.0

Comments

@KatharinaBuelow
Copy link

Generic Issue

  • xclim version: 0.44.0
  • Python version: 3.10
  • Operating System: linux

Description

We calculated snow cover duration for CORDEX data.

We noticed number of days can be both nan and 0 .
The calculation leads to nan if all values are equal 0.
The calculation leads to 0 if at least one value is greater than 0 but all values are below threshold.
In both cases we expected a 0.
Is there a special purpose for this differentiation?

Code of Conduct

  • I agree to follow this project's Code of Conduct
@aulemahal
Copy link
Collaborator

Hi @KatharinaBuelow and thanks for the issue!

Indeed, I'm not sure why we would need this differentiation. We have code that ensures that NaN is returned if there are not enough valid values, but it seems that "> 0" test was added to it. Here :

xclim/xclim/indices/_threshold.py

Line 2072 in 0ce4d30

valid = at_least_n_valid(snd.where(snd > 0), n=1, freq=freq)

Maybe @coxipi knows ?

But we could remove that extra test indeed.

@aulemahal aulemahal mentioned this issue Oct 5, 2023
Merged
5 tasks
@Zeitsperre Zeitsperre added this to the v0.46.0 milestone Oct 10, 2023
aulemahal added a commit that referenced this issue Oct 13, 2023
@aulemahal
Snow season length is 0 if there is 0 snow - fix solar_zenith_angle t…
435ea09 
…ypo (#1492)

<!--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 #1491 and fixes #1493
- [x] Tests for the changes have been added (for bug fixes / features)
- [ ] (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?
`snd_season_length` and `snw_season_length` got implemented with the
same check as their `*_season_start` and `*_season_end` counter parts,
with a mask where : `not at_least_n_valid(snw.where(snw > 0), n=1,
freq=freq)`.

However, this means the output is NaN if all inputs are 0. This makes
sense in the DOY case (can't a have a start/end date if there's no
season), but it doesn't in the "length" case. Instead, I think (as does
the issue raiser) an all-0 snow timeseries simply means a season length
of 0. Thus, I removed the `.where(snw > 0)` part of the test for those
two indicators.

EDIT: I also slipped in another fix for
`xc.indices.helpers.cosine_of_solar_zenith_angle`. There was a typo in
the `xr.apply_ufunc` call.

### Does this PR introduce a breaking change?
Yes because it will change the output of two indicators, but I think
this is for the best. The previous behaviour was not documented as such
in the doc, I believe the new one is actually more intuitive. No need
for a warning, IMO.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.46.0
Development

Successfully merging a pull request may close this issue.

Snow season length is 0 if there is 0 snow - fix solar_zenith_angle typo Ouranosinc/xclim
3 participants
@Zeitsperre @KatharinaBuelow @aulemahal