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

Update SEVIRI ICARE reader to properly use dask. #2103

Merged
merged 7 commits into from
May 6, 2022

Conversation

simonrp84
Copy link
Member

The iCARE/SEVIRI reader was not daskified, data was being stored as numpy arrays. This raised an error when trying to create a natural_color image as the Rayleigh correction code was expecting dask arrays.

This PR updates the reader so that the data is stored as a dask array.

@simonrp84 simonrp84 requested review from djhoese and mraspaud as code owners May 5, 2022 12:11
@pnuu pnuu added the bug label May 5, 2022
@mraspaud
Copy link
Member

mraspaud commented May 5, 2022

This is a reboot of #1758

@mraspaud
Copy link
Member

mraspaud commented May 5, 2022

Looks good, anything you can add to the tests to make sure the same error doesn't reappear in the future?

@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #2103 (0d1fb5e) into main (56eb21b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2103   +/-   ##
=======================================
  Coverage   93.92%   93.92%           
=======================================
  Files         283      283           
  Lines       42715    42722    +7     
=======================================
+ Hits        40120    40127    +7     
  Misses       2595     2595           
Flag Coverage Δ
behaviourtests 4.70% <0.00%> (-0.01%) ⬇️
unittests 94.48% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
satpy/readers/seviri_l1b_icare.py 90.19% <100.00%> (ø)
satpy/tests/reader_tests/test_seviri_l1b_icare.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56eb21b...0d1fb5e. Read the comment docs.

@pnuu
Copy link
Member

pnuu commented May 5, 2022

The only thing I can think of adding to tests would be the use of satpy.tests.utils import CustomScheduler and reading some simulated data and checking there are no computes. Not sure if that's feasible or not. See https://github.com/pytroll/satpy/blob/main/satpy/tests/test_composites.py#L1305-L1308 for an example of the usage.

Copy link
Member

@pnuu pnuu left a comment

Choose a reason for hiding this comment

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

LGTM

@coveralls
Copy link

coveralls commented May 5, 2022

Coverage Status

Coverage increased (+0.0009%) to 94.422% when pulling 0d1fb5e on simonrp84:icare_daskify into 56eb21b on pytroll:main.

@simonrp84
Copy link
Member Author

Would something like this be appropriate?

    def test_nocompute(self):
        """Test that dask does not compute anything in the reader itself."""
        from satpy.tests.utils import CustomScheduler
        import dask
        with dask.config.set(scheduler=CustomScheduler(max_computes=0)):
            r = load_reader(self.reader_configs)
            loadables = r.select_files_from_pathnames([
                'GEO_L1B-MSG1_2004-12-29T12-15-00_G_VIS08_V1-04.hdf'
            ])
            r.create_filehandlers(loadables)
            r.load(['VIS008'])

@mraspaud
Copy link
Member

mraspaud commented May 5, 2022

That looks about right! Just test this before and after your changes to make sure it works

@simonrp84
Copy link
Member Author

Ok, here we go, dask compute test added.

In adding this test I also discovered that the original tests were using pure numpy arrays rather than dask arrays - that's fixed now too.

With this new test, the pre-PR reader code fails, but the post-PR code passes.

Copy link
Member

@sfinkens sfinkens left a comment

Choose a reason for hiding this comment

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

LGTM, just one question.

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! I'll just subscribe to @sfinkens 's question saw now it was answered already

@mraspaud
Copy link
Member

mraspaud commented May 6, 2022

pre-commit.ci autofix

@mraspaud mraspaud merged commit e9f5317 into pytroll:main May 6, 2022
@simonrp84 simonrp84 deleted the icare_daskify branch August 22, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants