Fix args of bucket_sum and bucket_avg resampler

[ameraner]

This PR fixes the bucket_sum and bucket_avg resampler in Satpy, adapting for the changes in pyresample's PR pytroll/pyresample#319 .

On the side, it also fixes a small typo in the readers docs.

• Tests added
• Passes flake8 satpy
• Fully documented

[ameraner]

The changes in Pyresample that call for this PR have not been released yet. Should we wait to merge this PR until the new Pyresample release comes out, or how do we deal with these situations?

[codecov]

Codecov Report

Merging #1539 (cb12ff5) into master (5504ea4) will increase coverage by 0.30%.
The diff coverage is 98.96%.

@@            Coverage Diff             @@
##           master    #1539      +/-   ##
==========================================
+ Coverage   92.54%   92.85%   +0.30%     
==========================================
  Files         251      251              
  Lines       36801    37255     +454     
==========================================
+ Hits        34058    34593     +535     
+ Misses       2743     2662      -81     

Flag	Coverage Δ
behaviourtests	4.47% <6.18%> (-0.03%)	⬇️
unittests	92.99% <98.96%> (+0.30%)	⬆️

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

Impacted Files	Coverage Δ
satpy/resample.py	89.65% <95.23%> (+0.15%)	⬆️
satpy/tests/test_resample.py	99.67% <100.00%> (+0.34%)	⬆️
satpy/readers/satpy_cf_nc.py	97.56% <0.00%> (-1.04%)	⬇️
satpy/writers/__init__.py	88.83% <0.00%> (-0.11%)	⬇️
satpy/tests/test_writers.py	98.88% <0.00%> (-0.02%)	⬇️
satpy/scene.py	91.63% <0.00%> (-0.02%)	⬇️
satpy/tests/writer_tests/test_awips_tiled.py	98.23% <0.00%> (-0.02%)	⬇️
satpy/tests/test_utils.py	100.00% <0.00%> (ø)
satpy/modifiers/atmosphere.py	50.00% <0.00%> (ø)
satpy/tests/reader_tests/test_utils.py	100.00% <0.00%> (ø)
... and 15 more

---

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 5504ea4...cb12ff5. Read the comment docs.

[ghost]

DeepCode failed to analyze this pull request

Something went wrong despite trying multiple times, sorry about that.
Please comment this pull request with "Retry DeepCode" to manually retry, or contact us so that a human can look into the issue.

[djhoese]

Could you accept the mask_all_nans kwarg and if provided raise an exception to update pyresample. Or you could map it to the new kwarg and issue a warning. I'll leave that up to you.

[ameraner]

Hi @djhoese thanks for the review, I added a DeprecationWarning for the old kwarg.

There is still a bit of an issue with the Satpy/Pyresample versions here due to the renaming of the arguments:
The current Satpy master works with the latest Pyresample release, but not with the current Pyresample master.
After this PR, Satpy will work with the current Pyresample master, but not the latest Pyresample release.

So if we want Satpy to work with the current and older Pyresample releases, we would need to catch the TypeError (or inspect the function arguments) and pass the old kwarg with warnings. Otherwise we wait to merge this PR until the next Pyresample release and put a requirement. I suppose the first is preferred?

[djhoese]

Yeah not great is it. I was going to say let's leave it because I'm not a fan of inspecting functions for their arguments BUT what if you checked the version of pyresample at the top of the module? Like _pr_use_skipna = pyresample.__version__ >= 1.18.0? And then use that near the warning. If True then do the warning and reassign skipna = mask_all_nan and if False then only do the warning?

[ameraner]

@djhoese thanks for the hint, that sounds like a good idea! I tried to implement it in the last commit. To compare the versions I had to import packaging, is that fine or is there a better way? Hope the code didn't get too ugly now. Are tests required for these changes?

[djhoese]

I think the way you did it is fine. I personally think I would have done the "more wrong" way of just string comparisons.

I had one issue with your new function and commented below.

[djhoese]

Looks like you have some linting/website issues which I've comment on, but otherwise I think the logic looks good. Thanks for looking it over again.

[djhoese]

LGTM but I also never use the bucket resampler. If @pnuu could review before a merge that would be appreciated.

[pnuu]

LGTM.