Skip to content

Fixed potential memory leak in local_solar_time #2356

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

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Mar 1, 2024

Description

See SciTools/iris#5767: wrapping large arguments, such as numpy arrays in a functools.partial is an example of how functions can be created that will make this into a sizeable memory leak. This fixes this by using all numpy arrays as positional arguments in dask.array.apply_gufunc.

Closes #2355


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@schlunma schlunma added the dask related to improvements using Dask label Mar 1, 2024
@schlunma schlunma added this to the v2.11.0 milestone Mar 1, 2024
@schlunma schlunma requested a review from bouweandela March 1, 2024 11:04
@schlunma schlunma self-assigned this Mar 1, 2024
Copy link

codecov bot commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.11%. Comparing base (2be264d) to head (72859ef).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2356      +/-   ##
==========================================
- Coverage   94.11%   94.11%   -0.01%     
==========================================
  Files         241      241              
  Lines       13381    13379       -2     
==========================================
- Hits        12594    12592       -2     
  Misses        787      787              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bouweandela
Copy link
Member

bouweandela commented Mar 7, 2024

Thanks @schlunma Code looks good to me. I also did a test run. I did notice some strange vertical lines with very low values in them though. Here is the recipe:

datasets:
  - {dataset: ERA5, project: native6, tier: 3, type: reanaly}

preprocessors:
  local_solar_time:
    local_solar_time:

diagnostics:

  map:
    description: Global map of temperature in January 2000.
    themes:
      - phys
    realms:
      - atmos
    variables:
      pr:
        mip: E1hr
        preprocessor: local_solar_time
        timerange: 19900102/P2D
        caption: |
          Global map of {long_name} in January 2000 according to {dataset}.
    scripts:
      script1:
        script: examples/diagnostic.py
        quickplot:
          plot_type: pcolormesh
          cmap: Reds

and the plot (plotted time step 23), the white lines have values of order 1e-18:
native6_ERA5_reanaly_v1_E1hr_pr_19900102-P2D

This does not appear to be a new issue though, it also happens with the main branch.

@schlunma
Copy link
Contributor Author

schlunma commented Mar 7, 2024

Hmm, when I was testing this I've never seen these weird lines. I think these are missing values, not very small values. My guess is that it's somehow related to rounding, which leads to some longitude/time combinations having missing values. When this dataset is regridded to a course grid, these lines are gone.

Unfortunately I don't have the time to investigate this further, but this also shouldn't be a big issue to be honest, except if someone is interested in only these longitudes bands individually.

@schlunma schlunma merged commit 5ae3240 into main Mar 7, 2024
@schlunma schlunma deleted the dask_memory_leak branch March 7, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dask related to improvements using Dask
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential memory leak in local solar time preprocessor
2 participants