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

"parallel" -> "parallelized" #1493

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

"parallel" -> "parallelized" #1493

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

Comments

@KatharinaBuelow
Copy link

Generic Issue

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

Description

We found a spelling mistake:
https://github.com/Ouranosinc/xclim/blob/master/xclim/indices/helpers.py#L299
"parallel" -> "parallelized"

here it is fine:
https://github.com/Ouranosinc/xclim/blob/master/xclim/indices/generic.py#L993

Code of Conduct

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

Vielen dank, I'll slip a fix in my other PR!

@Zeitsperre Zeitsperre added this to the v0.46.0 milestone Oct 10, 2023
aulemahal added a commit that referenced this issue Oct 13, 2023
…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
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants