Add default consolidated flag for to_zarr #855

lsetiawan · 2022-10-14T20:55:46Z

Overview

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

lsetiawan · 2022-10-14T20:57:18Z

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

codecov-commenter · 2022-10-14T21:53:43Z

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%)`	⬇️

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%)`	⬇️
echopype/echodata/echodata.py	`78.50% <75.00%> (-2.69%)`	⬇️
echopype/convert/api.py	`84.72% <100.00%> (ø)`
echopype/echodata/zarr_combine.py	`95.96% <100.00%> (+0.07%)`	⬆️
echopype/metrics/summary_statistics.py	`0.00% <0.00%> (-96.43%)`	⬇️
echopype/calibrate/ecs_parser.py	`0.00% <0.00%> (-95.50%)`	⬇️
echopype/calibrate/calibrate_ek.py	`13.02% <0.00%> (-80.99%)`	⬇️
echopype/preprocess/noise_est.py	`21.05% <0.00%> (-73.69%)`	⬇️
... and 14 more

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

echopype/echodata/zarr_combine.py

b-reyes

@lsetiawan I am done reviewing this PR. From what I can see, It looks like you hit all of the to_zarr statements. There are a couple of places where consolidated should be removed because the consolidation has already occurred.

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?

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?

echopype/echodata/zarr_combine.py

lsetiawan · 2022-10-14T22:50:26Z

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 · 2022-10-14T22:55:13Z

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 · 2022-10-14T23:06:41Z

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 · 2022-11-14T04:55:20Z

@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.

echopype/echodata/zarr_combine.py

lsetiawan · 2022-12-06T19:42:25Z

@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 😬

echopype/tests/echodata/test_echodata.py

echopype/tests/echodata/test_zarr_combine.py

echopype/tests/echodata/utils.py

b-reyes · 2022-12-12T20:07:12Z

@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.

echopype/tests/echodata/utils.py

Co-authored-by: b-reyes <53541061+b-reyes@users.noreply.github.com>

for more information, see https://pre-commit.ci

lsetiawan · 2022-12-12T22:11:34Z

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

echopype/echodata/zarr_combine.py

echopype/tests/echodata/utils.py

b-reyes · 2022-12-12T22:59:57Z

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

Co-authored-by: b-reyes <53541061+b-reyes@users.noreply.github.com>

lsetiawan · 2022-12-13T17:34:23Z

@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 · 2022-12-13T18:17:24Z

@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 · 2022-12-13T18:56:06Z

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 · 2022-12-13T19:05:33Z

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

Great!

* Add default consolidated flag for to_zarr * Add docstring for kwargs * Expand the arguments for to_netcdf and to_zarr * Add test for to_zarr with mock echodata * Modify combine to consolidate at the end * Add zarr combine consolidated test * Remove output_path for mock echodata. * Add NMEA and dynamic expected_groups * Update echopype/tests/echodata/test_echodata.py Co-authored-by: b-reyes <53541061+b-reyes@users.noreply.github.com> * Add explicit False for delayed write_to_file * Duplicate ek60_test_data fixtures for future usage * Add check_consolidated func and refactor * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update echopype/echodata/zarr_combine.py Co-authored-by: b-reyes <53541061+b-reyes@users.noreply.github.com> * Add small docstring for check_consolidated Co-authored-by: b-reyes <53541061+b-reyes@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Add default consolidated flag for to_zarr

0682392

lsetiawan requested a review from b-reyes October 14, 2022 20:56

lsetiawan added the enhancement This makes echopype better label Oct 14, 2022

lsetiawan added this to the 0.6.3 milestone Oct 14, 2022

b-reyes reviewed Oct 14, 2022

View reviewed changes

echopype/echodata/zarr_combine.py Outdated Show resolved Hide resolved

b-reyes reviewed Oct 14, 2022

View reviewed changes

echopype/echodata/zarr_combine.py Outdated Show resolved Hide resolved

b-reyes reviewed Oct 14, 2022

View reviewed changes

echopype/echodata/zarr_combine.py Outdated Show resolved Hide resolved

b-reyes reviewed Oct 14, 2022

View reviewed changes

echopype/echodata/zarr_combine.py Show resolved Hide resolved

b-reyes suggested changes Oct 14, 2022

View reviewed changes

lsetiawan commented Oct 14, 2022

View reviewed changes

echopype/echodata/zarr_combine.py Outdated Show resolved Hide resolved

lsetiawan commented Oct 14, 2022

View reviewed changes

echopype/echodata/zarr_combine.py Outdated Show resolved Hide resolved

lsetiawan modified the milestones: 0.6.3, 0.6.4 Oct 14, 2022

lsetiawan commented Dec 5, 2022

View reviewed changes

echopype/echodata/zarr_combine.py Outdated Show resolved Hide resolved

lsetiawan commented Dec 5, 2022

View reviewed changes

echopype/echodata/zarr_combine.py Outdated Show resolved Hide resolved

lsetiawan commented Dec 5, 2022

View reviewed changes

echopype/echodata/zarr_combine.py Outdated Show resolved Hide resolved

lsetiawan added 4 commits December 5, 2022 13:48

Add docstring for kwargs

1ab8c16

Expand the arguments for to_netcdf and to_zarr

28d2236

Add test for to_zarr with mock echodata

3f02ee8

Modify combine to consolidate at the end

8200c07

lsetiawan commented Dec 6, 2022

View reviewed changes

echopype/echodata/zarr_combine.py Show resolved Hide resolved

lsetiawan added 2 commits December 6, 2022 11:34

Add zarr combine consolidated test

6f2b51b

Merge branch 'dev' into consolidated

4e60548

b-reyes reviewed Dec 12, 2022

View reviewed changes

echopype/tests/echodata/test_echodata.py Outdated Show resolved Hide resolved

b-reyes reviewed Dec 12, 2022

View reviewed changes

echopype/tests/echodata/test_zarr_combine.py Show resolved Hide resolved

b-reyes reviewed Dec 12, 2022

View reviewed changes

echopype/tests/echodata/utils.py Show resolved Hide resolved

b-reyes reviewed Dec 12, 2022

View reviewed changes

echopype/tests/echodata/utils.py Outdated Show resolved Hide resolved

b-reyes reviewed Dec 12, 2022

View reviewed changes

echopype/tests/echodata/utils.py Show resolved Hide resolved

lsetiawan commented Dec 12, 2022

View reviewed changes

echopype/tests/echodata/utils.py Outdated Show resolved Hide resolved

lsetiawan commented Dec 12, 2022

View reviewed changes

echopype/tests/echodata/utils.py Outdated Show resolved Hide resolved

lsetiawan and others added 7 commits December 12, 2022 12:14

Remove output_path for mock echodata.

4093d24

Add NMEA and dynamic expected_groups

e6c8d67

Update echopype/tests/echodata/test_echodata.py

b715ed3

Co-authored-by: b-reyes <53541061+b-reyes@users.noreply.github.com>

Add explicit False for delayed write_to_file

ab6bb12

Duplicate ek60_test_data fixtures for future usage

354f282

Add check_consolidated func and refactor

752bff5

[pre-commit.ci] auto fixes from pre-commit.com hooks

ccd4465

for more information, see https://pre-commit.ci

b-reyes reviewed Dec 12, 2022

View reviewed changes

echopype/echodata/zarr_combine.py Outdated Show resolved Hide resolved

b-reyes reviewed Dec 12, 2022

View reviewed changes

echopype/tests/echodata/utils.py Show resolved Hide resolved

b-reyes reviewed Dec 12, 2022

View reviewed changes

echopype/tests/echodata/utils.py Outdated Show resolved Hide resolved

lsetiawan and others added 2 commits December 12, 2022 15:07

Update echopype/echodata/zarr_combine.py

b6fbbaa

Co-authored-by: b-reyes <53541061+b-reyes@users.noreply.github.com>

Add small docstring for check_consolidated

bf1a5fa

b-reyes approved these changes Dec 13, 2022

View reviewed changes

lsetiawan merged commit f3ced32 into OSOceanAcoustics:dev Dec 13, 2022

lsetiawan deleted the consolidated branch December 13, 2022 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default consolidated flag for to_zarr #855

Add default consolidated flag for to_zarr #855

lsetiawan commented Oct 14, 2022

lsetiawan commented Oct 14, 2022

codecov-commenter commented Oct 14, 2022 •

edited

Loading

b-reyes left a comment

lsetiawan commented Oct 14, 2022

b-reyes commented Oct 14, 2022

lsetiawan commented Oct 14, 2022

leewujung commented Nov 14, 2022

lsetiawan commented Dec 6, 2022 •

edited

Loading

b-reyes commented Dec 12, 2022

lsetiawan commented Dec 12, 2022

b-reyes commented Dec 12, 2022

lsetiawan commented Dec 13, 2022

b-reyes commented Dec 13, 2022

lsetiawan commented Dec 13, 2022

b-reyes commented Dec 13, 2022

Add default consolidated flag for to_zarr #855

Add default consolidated flag for to_zarr #855

Conversation

lsetiawan commented Oct 14, 2022

Overview

lsetiawan commented Oct 14, 2022

codecov-commenter commented Oct 14, 2022 • edited Loading

Codecov Report

b-reyes left a comment

Choose a reason for hiding this comment

lsetiawan commented Oct 14, 2022

b-reyes commented Oct 14, 2022

lsetiawan commented Oct 14, 2022

leewujung commented Nov 14, 2022

lsetiawan commented Dec 6, 2022 • edited Loading

b-reyes commented Dec 12, 2022

lsetiawan commented Dec 12, 2022

b-reyes commented Dec 12, 2022

lsetiawan commented Dec 13, 2022

b-reyes commented Dec 13, 2022

lsetiawan commented Dec 13, 2022

b-reyes commented Dec 13, 2022

codecov-commenter commented Oct 14, 2022 •

edited

Loading

lsetiawan commented Dec 6, 2022 •

edited

Loading