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

cast high to float in jitter #1864

Merged
merged 8 commits into from
Aug 5, 2024
Merged

cast high to float in jitter #1864

merged 8 commits into from
Aug 5, 2024

Conversation

juliettelavoie
Copy link
Contributor

@juliettelavoie juliettelavoie commented Aug 2, 2024

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

  • I am casting high to be a float, not an array. In the new version of dask, if high is an array, there are memory problems and we are unable to run jitter_under on the ESPO regions.
  • I see that it was made explictly an array in mypy compliance #1721. @Zeitsperre do you remember why ?

Does this PR introduce a breaking change?

no

Other information:

This is only a problem when using dask.

@juliettelavoie juliettelavoie changed the title cast to high to float in jitter cast high to float in jitter Aug 2, 2024
@github-actions github-actions bot added the sdba Issues concerning the sdba submodule. label Aug 2, 2024
@aulemahal
Copy link
Collaborator

aulemahal commented Aug 2, 2024

The docstring is a bit ambiguous as it says "a quantity with units". In the real xclim, this would mean that DataArrays are accepted. But we didn't use the Quantified annotation here.

@RondeauG, to your knowledge, would there ever be a use for have the jitter limits change along a given dimension ? (Like a specific jitter for each latitudes).

My guess is no, so I suggest replacing the np.array cast above by float. And do this for all parameters. (lower + upper, + the other two functions).

@Zeitsperre
Copy link
Collaborator

Many of the changes I made in that PR were to pin down what exactly the expectations were for each function. Like @aulemahal says, some function docstrings were very ambiguous. The Quantified type still has some inconsistencies in the code base, so I vaguely recall setting that to an array, as I was certain that str was not supported.

@github-actions github-actions bot added the approved Approved for additional tests label Aug 5, 2024
@coveralls
Copy link

coveralls commented Aug 5, 2024

Coverage Status

coverage: 89.262%. first build
when pulling ac342ce on fix-jitter
into 4226f57 on main.

@juliettelavoie juliettelavoie merged commit f475729 into main Aug 5, 2024
19 checks passed
@juliettelavoie juliettelavoie deleted the fix-jitter branch August 5, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests sdba Issues concerning the sdba submodule.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants