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

Fix unnecessary Dask compute()s in NDVIHybridGreen compositor #2623

Merged
merged 6 commits into from
Nov 2, 2023

Conversation

pnuu
Copy link
Member

@pnuu pnuu commented Nov 1, 2023

The NDVIHybridGreenCompositor was using da.where() on a DataArray to replace values outside limits causing unnecessary computations. This PR replaces these two calls with a DataArray.clip() and adds checks in the tests that there is only one computation and only when it is requested.

@pnuu pnuu added bug enhancement code enhancements, features, improvements component:compositors labels Nov 1, 2023
@pnuu pnuu self-assigned this Nov 1, 2023
@pnuu pnuu changed the title Fix unnecessary Dask 'compute()s in NDVIHybridGreen` compositor Fix unnecessary Dask compute()s in NDVIHybridGreen compositor Nov 1, 2023
Copy link

codecov bot commented Nov 1, 2023

Codecov Report

Merging #2623 (dd2c879) into main (86c075a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2623   +/-   ##
=======================================
  Coverage   95.18%   95.18%           
=======================================
  Files         354      354           
  Lines       51270    51280   +10     
=======================================
+ Hits        48803    48813   +10     
  Misses       2467     2467           
Flag Coverage Δ
behaviourtests 4.24% <0.00%> (-0.01%) ⬇️
unittests 95.81% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
satpy/composites/spectral.py 100.00% <100.00%> (ø)
satpy/tests/compositor_tests/test_spectral.py 100.00% <100.00%> (ø)

@strandgren
Copy link
Collaborator

Thanks @pnuu. Such a small change to make the code shorter, cleaner and much more efficient :D Nice addition to the tests as well!

@coveralls
Copy link

coveralls commented Nov 1, 2023

Pull Request Test Coverage Report for Build 6721469074

  • 23 of 23 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.759%

Totals Coverage Status
Change from base Build 6663415040: 0.0%
Covered Lines: 48933
Relevant Lines: 51100

💛 - Coveralls

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. One other check you could add is seeing if the computed numpy array from .values or .data.compute() has the same dtype as the DataArray. You could force the input data to be 32-bit floats and make sure it is np.float32 in the output since we rarely want data to be upcast to 64-bit. If the DataArray and the numpy array have different dtypes that's usually a sign that a map_blocks is doing things against it's interface.

If that seems pointless here then feel free to merge.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@pnuu pnuu merged commit d59e467 into pytroll:main Nov 2, 2023
18 of 19 checks passed
@pnuu pnuu deleted the bugfix-hybrid-green-compute branch November 2, 2023 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component:compositors enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NDVI hybrid green correction triggers early dask computations
5 participants