Ouranosinc / xclim / 6512018602

90%

main: 92%

push

github

Snow season length is 0 if there is 0 snow - fix solar_zenith_angle typo (#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.

2 of 2 new or added lines in 1 file covered. (100.0%)

8393 of 9284 relevant lines covered (90.4%)

4.37 hits per line