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

feat: pin netcdf to >1.6 and add explicit encoding [all tests ci] #1112

Merged
merged 6 commits into from
Aug 4, 2023

Conversation

lsetiawan
Copy link
Member

Overview

This PR unpins the netCDF4 version and set it to >1.6 so that we are getting the latest. Also, the set_netcdf_encodings utils has been updated so that we set zlib=False for strings... it looks like the older netcdf4 is already doing this under the hood.

@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2023

Codecov Report

Merging #1112 (90f4a45) into dev (4b3c70e) will increase coverage by 0.03%.
Report is 3 commits behind head on dev.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #1112      +/-   ##
==========================================
+ Coverage   78.14%   78.17%   +0.03%     
==========================================
  Files          65       65              
  Lines        6232     6241       +9     
==========================================
+ Hits         4870     4879       +9     
  Misses       1362     1362              
Flag Coverage Δ
unittests 78.17% <100.00%> (+0.03%) ⬆️

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

Files Changed Coverage Δ
echopype/utils/coding.py 90.42% <100.00%> (+0.77%) ⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@emiliom
Copy link
Collaborator

emiliom commented Aug 4, 2023

I've reviewed the PR, and it looks good. Next, I'll run it locally.

@emiliom
Copy link
Collaborator

emiliom commented Aug 4, 2023

Everything seems to be working great!

Here's the encoding on Beam_group1.channel, with a conda environment built with netcdf4 1.6:

In [7]: ds16 = xr.open_dataset(
   'with_netcdf4v16/Summer2017-D20170615-T190214.nc', group='Sonar/Beam_group1', decode_cf=False
)

In [8]: ds16.channel.encoding
Out[8]:
{'zlib': False,
 'shuffle': False,
 'complevel': 0,
 'fletcher32': False,
 'contiguous': True,
 'chunksizes': None,
 'source': '/home/mayorga/.echopype/temp_output/with_netcdf4v16/Summer2017-D20170615-T190214.nc',
 'original_shape': (3,),
 'dtype': str}

With netcdf4 1.5, the encoding is identical.

File sizes are trivially different, with the netcdf4 1.6 nc file being 10 bytes smaller out of 1.4 MB.

@lsetiawan lsetiawan merged commit eba3cca into OSOceanAcoustics:dev Aug 4, 2023
3 checks passed
@lsetiawan lsetiawan deleted the nc4_update branch August 4, 2023 17:34
@leewujung
Copy link
Member

Thanks @lsetiawan @emiliom !

To close the loop: the changes boiled down to set string type encoding with "zlib" : False. It seems this PR addresses everything in #988. Can you confirm?

@emiliom
Copy link
Collaborator

emiliom commented Aug 12, 2023

To close the loop: the changes boiled down to set string type encoding with "zlib" : False.

Yup.

It seems this PR addresses everything in #988. Can you confirm?

Ultimately, yes. The core motivation for that issue has been fully addressed. But on re-reading, there are some subtleties that I'll comment on there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants