Add function to interpolate location to calibrated dataset

[leewujung]

This addresses #725 .

Tasks

• add function to utils.common (temporary position until we decide where these higher-product-level utilities would go)
• [ ] add tests to Sv and MVBS data

[leewujung]

@lsetiawan : When I added the typing of EchoData somehow I am getting circular import error! Do you have ideas on where that may come from? I thought I have exactly the same thing (from ..echodata import EchoData) as what we had in calibrate/api.py. Help!

[leewujung]

@emiliom : is this appropriate (for now)?

[lsetiawan]

When I added the typing of EchoData somehow I am getting circular import error! Do you have ideas on where that may come from? I thought I have exactly the same thing (from ..echodata import EchoData) as what we had in calibrate/api.py. Help!

Since common is imported under the utils __init__.py as such:

echopype/echopype/utils/__init__.py

Lines 1 to 5 in 27a9b82

	from .common import swap_dims_channel_frequency
	__all__ = [
	"swap_dims_channel_frequency",
	]

within echodata.py we have a utils import so it will try to import the function from common, which creates the circular import:

echopype/echopype/echodata/echodata.py

Line 18 in 27a9b82

from ..utils.coding import set_encodings

This is the issue. Probably shouldn't be importing EchoData class within utils common since, it is a "common" module that can be used in any of the other modules. Best to reserve this to module to not pull from other modules.

[lsetiawan]

Should this be in the preprocess module instead? After all it enhances the spatial coherence of the data? 😄

echopype/echopype/preprocess/api.py

Line 2 in 12693a7

Functions for enhancing the spatial and temporal coherence of data.

[leewujung]

Oh that is very true! I agree with this and will move it.

[leewujung]

I guess I had forgotten that I've written those sentences...

[leewujung]

Probably shouldn't be importing EchoData class within utils common since, it is a "common" module that can be used in any of the other modules. Best to reserve this to module to not pull from other modules.

I agree with this! I wonder where these functions can go (swapping freq/channel, adding lat/lon, adding depth, etc -- basically higher level processing functions). I sticked common.py under utils because we hadn't discussed this, but now with this circular import/logic issue, we should move it out to something that is more sensible. Maybe these belong in preprocess also as you pointed out above? preprocess.data_coherence (just made up the name, in my mind these are functions that bring coherence across different "types" of data into the Sv dataset)

[lsetiawan]

I agree with this! I wonder where these functions can go (swapping freq/channel, adding lat/lon, adding depth, etc -- basically higher level processing functions).

I think higher level functions that can users will/can use should be in the preprocess module since it is a "preprocessing" step. I think of the utils module more for pure functions that we need for our purposes for either I/O, data formatting utilities, or pure calculations such as the sound speed stuff. more than information manipulations (processing).

[codecov-commenter]

Codecov Report

Merging #749 (44fb8e1) into dev (c27cda1) will decrease coverage by 62.58%.
The diff coverage is 38.46%.

@@             Coverage Diff             @@
##              dev     #749       +/-   ##
===========================================
- Coverage   82.33%   19.74%   -62.59%     
===========================================
  Files          46       48        +2     
  Lines        4240     4264       +24     
===========================================
- Hits         3491      842     -2649     
- Misses        749     3422     +2673     

Flag	Coverage Δ
unittests	19.74% <38.46%> (-62.59%)	⬇️

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

Impacted Files	Coverage Δ
echopype/consolidate/api.py	27.27% <27.27%> (ø)
echopype/__init__.py	100.00% <100.00%> (ø)
echopype/consolidate/__init__.py	100.00% <100.00%> (ø)
echopype/metrics/summary_statistics.py	0.00% <0.00%> (-96.43%)	⬇️
echopype/calibrate/ecs_parser.py	0.00% <0.00%> (-95.50%)	⬇️
echopype/utils/uwa.py	6.12% <0.00%> (-87.76%)	⬇️
...echodata/sensor_ep_version_mapping/v05x_to_v06x.py	13.33% <0.00%> (-86.25%)	⬇️
echopype/convert/set_groups_ek80.py	12.00% <0.00%> (-84.58%)	⬇️
echopype/convert/set_groups_ad2cp.py	14.85% <0.00%> (-84.00%)	⬇️
echopype/calibrate/calibrate_ek.py	12.50% <0.00%> (-81.79%)	⬇️
... and 28 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[emiliom]

I'm submitting a PR (leewujung#6) to your PR for your evaluation, with the following changes:

• Support for fixed-location case (a single lat-lon coordinate in the Platform group), by propagating that constant lat-lon pair along the ping_time dimension
• Add tests for missing or all-nan Platform coordinate variables
• Minor cosmetic tweaks

See what you think. Otherwise, I think it's ready to go. FYI I did a manual, visual evaluation of the lat-lon interpolation, comparing the results against the original lat-lon data. It looks good.

[leewujung]

Add tests for missing or all-nan Platform coordinate variables

This is kinda of a confusion/misnomer: what's added was the mechanism to test for missing or all-nan Platform coordinate variables, but not "tests" for them. We still need tests for these.

I will open up a new issue for completing the more comprehensive tests, and delegate that to someone, so that I can focus on adding the actual functions.