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

Drop default compression in CF Writer #2390

Merged
merged 11 commits into from Mar 6, 2023

Conversation

sfinkens
Copy link
Member

@sfinkens sfinkens commented Feb 14, 2023

Drop default compression in CF Writer and delegate this to the user (via encoding keyword argument or dataset attribute). This closes #2244. Also, update documentation to use the new compression keyword instead of zlib, which closes #2386 .

I played around a bit with different combinations of backend versions (netCDF4, libnetcdf, xarray) and noticed that the keyword choice also depends on the underlying libnetcdf. If libnetcdf<4.9.0 only zlib works and compression is silently ignored. This is the case in our CI environment whereeccodes currently requires libnetcdf-4.8.

In order to make users aware of that, I added a little section in the documentation and issued a user warning if the backend versions don't match.

Waiting for #2391 to fix flake8 errors.

  • Tests added
  • Fully documented

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.

Can't say I'm happy with the amount of version checks needed but that's not your/our fault. Too bad it isn't easier.

Thanks for fixing this up. I had a couple comments, but otherwise looks pretty good.

satpy/writers/cf_writer.py Outdated Show resolved Hide resolved
satpy/writers/cf_writer.py Outdated Show resolved Hide resolved
satpy/writers/cf_writer.py Outdated Show resolved Hide resolved
satpy/writers/cf_writer.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #2390 (d54accf) into main (5f2e3a1) will increase coverage by 0.02%.
The diff coverage is 98.93%.

@@            Coverage Diff             @@
##             main    #2390      +/-   ##
==========================================
+ Coverage   94.71%   94.73%   +0.02%     
==========================================
  Files         329      329              
  Lines       48739    48790      +51     
==========================================
+ Hits        46161    46223      +62     
+ Misses       2578     2567      -11     
Flag Coverage Δ
behaviourtests 4.41% <0.00%> (+<0.01%) ⬆️
unittests 95.36% <98.93%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
satpy/tests/writer_tests/test_cf.py 99.60% <98.70%> (-0.12%) ⬇️
satpy/writers/cf_writer.py 95.63% <100.00%> (+0.47%) ⬆️
satpy/resample.py 79.96% <0.00%> (+0.65%) ⬆️
satpy/_compat.py 69.38% <0.00%> (+4.08%) ⬆️
satpy/_config.py 95.55% <0.00%> (+4.44%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@coveralls
Copy link

Coverage Status

Coverage: 95.326% (+0.007%) from 95.319% when pulling d54accf on sfinkens:cf-writer-encoding into 5f2e3a1 on pytroll:main.

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.41.0 milestone Mar 6, 2023
@mraspaud mraspaud merged commit 8f30b2b into pytroll:main Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants