Added support for consolidate subpackage functions to accept both in-memory or stored datasets

[praneethratna]

Addresses #1129, #1237 and now consolidate subpackage functions can accept both in-memory or stored datasets.

CC @leewujung

[codecov-commenter]

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (7679b96) 83.29% compared to head (bdee1cc) 62.43%.
Report is 14 commits behind head on dev.

Files	Patch %	Lines
echopype/utils/io.py	55.55%	8 Missing ⚠️
echopype/consolidate/api.py	91.66%	2 Missing ⚠️
echopype/echodata/echodata.py	66.66%	1 Missing ⚠️
echopype/mask/api.py	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##              dev    #1216       +/-   ##
===========================================
- Coverage   83.29%   62.43%   -20.87%     
===========================================
  Files          64       24       -40     
  Lines        5675     1621     -4054     
===========================================
- Hits         4727     1012     -3715     
+ Misses        948      609      -339     

Flag	Coverage Δ
unittests	62.43% <76.47%> (-20.87%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[leewujung]

Hey @praneethratna : Thanks for the PR! I only have pretty small comments and a question here.

While going through the changes, I noticed that the use of source_Sv_path does not make much sense () and that results in the mess in add_splitbeam_angle that source_Sv is separately validated outside of open_ds_da_ed_path. We should probably deal with it in a separate PR, to keep this one tidy.

[leewujung]

@praneethratna : Thanks for the changes. I have two tiny last suggestions (not sure why these didn't come to mind last time):

• rename open_ds_da_ed_path to open_source
• rename validate_source_ds_da_ed to validate_source

I think then we can merge this!

[praneethratna]

Hey @leewujung I have addressed the above comments and also included the changes for #1237 in this PR since they are based on these changes. Thanks!

[leewujung]

Hey @praneethratna : The last changes you made removed the part to save the added theta and phi variables to an existing dataset. Are you thinking that these variables would just be live in source_Sv?

Maybe we just remove the add_angle_to_ds function and put the content (with necessary modifications) in add_splitbeam_angle?

[praneethratna]

Hey @praneethratna : The last changes you made removed the part to save the added theta and phi variables to an existing dataset. Are you thinking that these variables would just be live in source_Sv?

Maybe we just remove the add_angle_to_ds function and put the content (with necessary modifications) in add_splitbeam_angle?

Hey @leewujung I think they should live, but anyway I have moved the content to add_splitbeam_angle itself and also removed the return_dataset parameter!

[leewujung]

Hey @leewujung I think they should live, but anyway I have moved the content to add_splitbeam_angle itself and also removed the return_dataset parameter!

Ah I see what the confusion may be. What I meant is that the functionality of saving the new variables phi and theta was removed in your changes, but we actually need the functionality (to update the on-disk dataset with the added variables phi and theta, while still being able to access them as angle_alongship and angle_athwartship).

How about we do the following:

• remove return_dataset
• add a new argument to_disk (default to True), where to_disk=True means that the angle will be saved to disk via mode="a" and the source_Sv dataset lazy-loaded back and returned, and to_disk=False means that the angle will just be added to source_Sv and returned but not saved to disk.

[leewujung]

@praneethratna : I put in a couple comments about changes needed in the logic of checking the combination to_disk and souce_Sv type. Thanks!

[leewujung]

@praneethratna : I found a bug in the latest change and commented on it. Should be pretty straightforward to fix.

For the testing case where to_disk=True, please also add the below to make sure that it is actually loading data from the temp zarr file (in which case the .data is a dask array, not a numpy array):

assert isinstance(source_Sv["angle_alongship"].data, dask.array.core.Array)
assert isinstance(source_Sv["angle_athwartship"].data, dask.array.core.Array)

Thanks!

[leewujung]

Awesome, thanks for the changes! I'll merge this now!