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

Cache slicing arrays in bilinear resampler #897

Merged
merged 20 commits into from
Sep 18, 2019

Conversation

pnuu
Copy link
Member

@pnuu pnuu commented Sep 6, 2019

For bilinear interpolation, this PR caches the slicing arrays (for data and masking) instead of the valid_input_index and index_array, from which the current version would always re-calculate these slicing arrays.

Also, instead of using da.from_zarr() to load the cache, direct use of zarr seems to make a big difference in speed. When/if someone makes the effort to completely daskify the bilinear resampling, this change might need to be reverted.

  • Tests added and test suite added to parent suite
  • Tests passed
  • Passes flake8 satpy
  • Fully documented

@pnuu pnuu self-assigned this Sep 6, 2019
@pnuu pnuu added component:resampling enhancement code enhancements, features, improvements optimization refactor labels Sep 6, 2019
@pnuu pnuu marked this pull request as ready for review September 11, 2019 10:54
@pnuu
Copy link
Member Author

pnuu commented Sep 11, 2019

I'm at a loss with the failing test. The same test works locally as expected, but not on either of Travis or Appveyor. Any ideas?

@mraspaud
Copy link
Member

mraspaud commented Sep 11, 2019

Could it be that the zarr file is already present due to some other test and that the conversion is skipped ? Just a wild guess...

@pnuu
Copy link
Member Author

pnuu commented Sep 11, 2019

The failing test has its own different cache_dir. They are created with tempfile.mkdtemp() in each test so they have different names.

@djhoese
Copy link
Member

djhoese commented Sep 11, 2019

The bottom of the failing tests (during the behavior tests) says that zarr isn't installed.

@pnuu
Copy link
Member Author

pnuu commented Sep 11, 2019

Hmph. I guess the CI services do not respect what's listed in setup.py. I'll add zarr to travis and appveyor configs. Thanks.

@pnuu
Copy link
Member Author

pnuu commented Sep 11, 2019

Nope, same failure.

@djhoese
Copy link
Member

djhoese commented Sep 11, 2019

Well the travis behavior tests are passing now. I'll see if I notice anything in the code.

@djhoese
Copy link
Member

djhoese commented Sep 11, 2019

Python 2.7 is failing because "FileExistsError" does not exist.

@codecov
Copy link

codecov bot commented Sep 12, 2019

Codecov Report

Merging #897 into master will increase coverage by 0.07%.
The diff coverage is 96.87%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #897      +/-   ##
=========================================
+ Coverage   84.83%   84.9%   +0.07%     
=========================================
  Files         171     171              
  Lines       25232   25299      +67     
=========================================
+ Hits        21405   21480      +75     
+ Misses       3827    3819       -8
Impacted Files Coverage Δ
satpy/readers/generic_image.py 93.1% <ø> (ø) ⬆️
satpy/tests/test_resample.py 97.71% <100%> (-0.51%) ⬇️
satpy/resample.py 92.77% <91.17%> (+2.91%) ⬆️

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 d0e1cd0...6c065a7. Read the comment docs.

@pnuu
Copy link
Member Author

pnuu commented Sep 12, 2019

It was indeed the filenames. The filenames I had created had slightly different kwargs than those used within the code.

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

@mraspaud mraspaud added this to the v0.17.0 milestone Sep 12, 2019
@coveralls
Copy link

coveralls commented Sep 16, 2019

Coverage Status

Coverage increased (+0.07%) to 84.904% when pulling 8035ba2 on pnuu:feature-bilinear-cache-slices into d0e1cd0 on pytroll:master.

@mraspaud mraspaud merged commit 34e4045 into pytroll:master Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants