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

Ensure inputs of cosine of solar zenith angle are chunked when needed #1542

Merged
merged 8 commits into from
Nov 30, 2023

Conversation

aulemahal
Copy link
Collaborator

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?

  • New function xc.core.utils._chunk_like to chunk a list of inputs according to one chunk dictionary. It also circumvents [Bug]: cannot chunk a DataArray that originated as a coordinate pydata/xarray#6204 by recreating DataArrays that where obtained from dimension coordinates.
  • Generalization of uses_dask so it can accept a list of objects.
  • Usage of _chunk_like to ensure the inputs of cosine_of_solar_zenith_angle are chunked when needed, in mean_radiant_temperature and potential_evapotranspiration.

The effect of this is simply that the cosine_of_solar_zenith_angle will be performed on blocks of the same size as in the original data, even though its inputs (the dimension coordinate) did not carry that information. Before this PR, the calculation was done as a single block, of the same size as the full array.

Does this PR introduce a breaking change?

No.

Other information:

Dask might warn something like PerformanceWarning: Increasing number of chunks by factor of NN. Where NN should be the number of chunks along the lat dimension, if present. That exactly what we want, so it's ok.

@aulemahal aulemahal requested a review from coxipi November 29, 2023 20:53
@github-actions github-actions bot added the indicators Climate indices and indicators label Nov 29, 2023
@aulemahal aulemahal changed the title Ensure inputs of solar cosine of zenith angle are chunked when needed Ensure inputs of cosine of solar zenith angle are chunked when needed Nov 29, 2023
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.

Very nice. I'm not sure if this would be appropriate, but I had the thought that cosine_of_solar_zenith_angle and extraterrestrial_solar_radiation could manage the chunking themselves by accepting an argument chunks. Since they only accept those coordinate arrays, seems like something that will happen all the time. But, perhaps, we don't want to do these kinds of operations at this low level. Anyways, just a thought I'm sharing since I had nothing more to say, LGTM.

@github-actions github-actions bot added the approved Approved for additional tests label Nov 29, 2023
@aulemahal
Copy link
Collaborator Author

@coxipi I think that idea is better indeed!

I changed to a chunks arguments in the cosine function itself. The user still needs to know to pass that argument, but it's much shorter!

@aulemahal
Copy link
Collaborator Author

@coxipi The previous CI run failed because of two bugs:

  1. I was chunking all the time, even when not needed. This is now fixed.
  2. SPI uses a function that was not implemented for dask arrays with xarray <= 2023.3.0 (ds.pr.groupby(...).first())
    Haha I think this was my suggestion...

And with python 3.8, xarray can't go over 2023.1.0, so SPI and SPEI will fail for dask arrays in python 3.8.

I suggest we do nothing about this as the plan is to drop 3.8 (and those older xarrays) in january.

@coveralls
Copy link

Coverage Status

coverage: 90.433% (-0.006%) from 90.439%
when pulling ce2c565 on fix-1536
into 6efc676 on master.

@aulemahal aulemahal merged commit cf4bf5a into master Nov 30, 2023
16 checks passed
@aulemahal aulemahal deleted the fix-1536 branch November 30, 2023 15: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.

Cosine of solar zenith angle takes way too much memory for long datasets
3 participants