Add default consolidated flag for to_zarr

[lsetiawan]

Overview

This PR adds a default of consolidated=True for to_zarr but also allows user to modify it, per conversation from #233.

[lsetiawan]

@b-reyes this is ready for review... I hope I covered all of the to_zarr appropriately. Thanks.

[codecov-commenter]

Codecov Report

Merging #855 (4e60548) into dev (b0b1490) will decrease coverage by 14.16%. The diff coverage is 73.33%.

@@             Coverage Diff             @@
##              dev     #855       +/-   ##
===========================================
- Coverage   78.54%   64.37%   -14.17%     
===========================================
  Files          53       54        +1     
  Lines        5112     5125       +13     
===========================================
- Hits         4015     3299      -716     
- Misses       1097     1826      +729     

Flag	Coverage Δ
unittests	64.37% <73.33%> (-14.17%)	:arrow_down:

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

Impacted Files	Coverage Δ
echopype/echodata/combine.py	70.00% <ø> (ø)
echopype/qc/api.py	0.00% <0.00%> (ø)
echopype/utils/io.py	68.24% <33.33%> (-16.90%)	:arrow_down:
echopype/echodata/echodata.py	78.50% <75.00%> (-2.69%)	:arrow_down:
echopype/convert/api.py	84.72% <100.00%> (ø)
echopype/echodata/zarr_combine.py	95.96% <100.00%> (+0.07%)	:arrow_up:
echopype/metrics/summary_statistics.py	0.00% <0.00%> (-96.43%)	:arrow_down:
echopype/calibrate/ecs_parser.py	0.00% <0.00%> (-95.50%)	:arrow_down:
echopype/calibrate/calibrate_ek.py	13.02% <0.00%> (-80.99%)	:arrow_down:
echopype/preprocess/noise_est.py	21.05% <0.00%> (-73.69%)	:arrow_down:
... and 14 more

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

[lsetiawan]

One thing that I noticed is that in functions such as to_zarr in echodata.py you provide docstrings for kwargs. This seems confusing to me. Is there a numpy doc way of specifying docstring parameters that are specifically for kwargs?

I think this was from the implicity of to_file function... we should probably change it so that we do list out all the kwargs. 😛

[b-reyes]

I think this was from the implicity of to_file function... we should probably change it so that we do list out all the kwargs. 😛

Yeah, I think that is best. If you think this is best too, could you create an issue for it and we can deal with it after this release?

[lsetiawan]

With these changes, have you tried to run a large number of files in combine_echodata? If not, can you please do that to make sure nothing has changed?

I won't have time to fully test this 🙈 Maybe just push this off to the next release since it's not an urgent issue.

[leewujung]

@lsetiawan @b-reyes : following up on this -- could you connect and make sure the remaining issues get resolved in this week? I think it would be good to add the required docstrings in this PR to not overcrowd the issue list.

[lsetiawan]

@b-reyes This PR is ready for another review. I've put in some tests in there to ensure that everything is consolidating properly. One of the biggest changes is that for your combine function, I put the consolidation at the very end so no metadata is missed, and default all the various private function calls to to_zarr to not consolidate. Also, sorry for the styling changes in there also! My auto pre-comit did that 😬

[b-reyes]

@lsetiawan I am done reviewing this. I had a minor comment on the consolidated portion and the rest of the comments were on the tests/mock EchoData. Please address these comments.

[lsetiawan]

@b-reyes Okay. I've implemented your suggestions. Let me know if I miss anything. Thanks!

[b-reyes]

@lsetiawan I am done reviewing. I only have minor remarks in regards to documentation/commenting.

[lsetiawan]

@b-reyes I've added more changes based on your comments. Ignore the RTD failure, for some reason it can't check out the changes... bizarre... All the other tests succeed.

[b-reyes]

@lsetiawan thank you very much for making those changes. All of my concerns have been addressed. Please go ahead and merge these changes in.

I am slightly worried about the RTD failure, but I believe we can resolve that after a merge, if necessary.

[lsetiawan]

I am slightly worried about the RTD failure, but I believe we can resolve that after a merge, if necessary.

The RTD failure had nothing to do with this PR, no stress 😄 See it all works: https://readthedocs.org/projects/echopype/builds/18907478/

[b-reyes]

The RTD failure had nothing to do with this PR, no stress 😄 See it all works: https://readthedocs.org/projects/echopype/builds/18907478/

Great!