Include water_level or range_offset in Sv dataset

[emiliom]

From @leewujung, #436 (comment):

One thing I noticed which we'll need to do something about (also requested in #259 ) is to include the addition of water_level in the Sv dataset (since right now even if users put in water_level=True if the input is an Sv dataset no water_level offset is added. A more accurate way to think about it is probably that we are actually plotting "depth" in the case when we have range_meters + water_level.

The more general description is that people are plotting the range offset from some platform. Let's discuss how that coordinate should be named... Hence I am wondering if water_level should be range_offset instead, so that it is more general.

Pinging @emiliom here since this is related to the whole convention and attribute conversation.

[b-reyes]

1. To allow for the inclusion of the water level in the Sv dataset, I propose that we make this an option in the calibrate function compute_Sv. For example, we could have the function parameter "include_range_offset" with a default value of False.

In regards to actually implementing this, I believe the simplest place to do this is in the if statement i.e. line 56 in the code below:

echopype/echopype/calibrate/api.py

Lines 54 to 58 in 17a86e4

	# Perform calibration
	if cal_type == "Sv":
	return cal_obj.compute_Sv(waveform_mode=waveform_mode, encode_mode=encode_mode)
	else:
	return cal_obj.compute_Sp(waveform_mode=waveform_mode, encode_mode=encode_mode)

this will allow us to skip duplicate code in the calibrated classes for each of the echosounders.

2. In the returned Xarray Dataset, I also propose that we call the data variable "range_offset" to allow for the generality previously mentioned.

[emiliom]

Hence I am wondering if water_level should be range_offset instead, so that it is more general.

I'm only now paying close attention to the convention context for this change (sorry!), and realizing that water_level is specified in the convention but range_offset isn't. That's also the case for the vers. 2 draft of the convention. While I see the logic for this generalization, @leewujung could you flesh out your argument for deviating from the convention?

[leewujung]

Sigh, yes, in commenting on your comment to #579 I just went back to the convention again and found the same thing. I'm copying my comment there below:

So, I just went back to check the convention and there is actually no range_offset in there but there is water_level. 😭 But there is also vertical_offset... 🤯 I am now very confused, both in how we (was that me?) arrived at changing things to range_offset (although I do think this is more general) and the relationship between the two.

In the convention:
water_level is

Distance from the origin of the platform coordinate system to the nominal water level measured along the z-axis of the platform coordinate system (positive values are below the origin). The distance between the nominal and actual water level is provided by vertical_offset.

vertical_offset is

Distance from the nominal water level to the actual water level measured along the z-axis of the platform coordinate system (positive values are when the actual water level is below the nominal water level). For ships and similar, this is called heave, but the concept applies equally well to underwater vehicle depth. This offset is applied at the position given by (MRU_offset_x, MRU_offset_y, MRU_offset_z).

I don't remember what my original argument was, but looking at this again, I think calling it range_offset is more general, because there is no guarantee that sonar systems/echosounders are pointed vertically (upward or downward), but there is probably always a need to specify how to convert the values in echo_range to something that makes sense in the context of data collection (like from echo_range to depth).

I think I understand why the convention was written this way, since it was very "hard-wired" to a ship-based sonar system (even though the above does try to accommodate the case for an underwater vehicle). But I guess I am leaning more toward the side that users should know what the variables mean in the sampling context.

@emiliom : What would you recommend as the right step?

[emiliom]

Hmm. After reading and rereading the definitions of water_level and vertical_offset, plus the Coordinate Systems chapter in SONAR-netCDF v1 (ch. 4), here are some observations -- some of which I'm sure are obvious, but it helps me to state them:

1. The platform coordinate system is the grounding for all other coordinates. The origin of that coordinate system is arbitrary.
2. There are 3 types of x-y-z offsets defined: position (GPS), MRU and transducer.
3. water_level and vertical_offset are variables that refer exclusively to the vertical (z) axis and are there to enable the generation of depth values relative to actual water level

The generalization to range_offset implies an along-range ("transducer orientation", in my layman's term) axis relative to the platform origin, or relative to actual or nominal water level (I can't remember which). But regardless of the orientation of the transducer, its offsets x-y-z relative to platform origin are already specified by the transducer offsets, and water_level and vertical_offset together provide that vertical referencing to either a constant water level (I assume that's what nominal water level is?) or actual, time varying water level. It's not clear to me what value the generalized, along-range range_offset actually provides when all the other offsets as well as water_level and vertical_offset are available. I think what's potentially missing is more clarity and specificity about the coordinate system for platforms other than ships. This is apparently being addressed in the convention v2 draft (see #517).

Where does this leave us? I'd put PR #579 on hold until we've had a chance to arrive at a clear consensus, including examining how echopype is handling the position, MRU and transducer offset variables, plus water_level and vertical_offset. Looking at a Hake survey converted file (from the HTML repr in the docs, here), I see that vertical_offset and MRU and position offset variables are missing, and transducer offset variables are found in the Beam group rather than the Platform group. So, we have work to do. But it's also looking like the change of water_level to range_offset might not be warranted.

[leewujung]

Great, thanks @emiliom, I think these discussions clear this up quite a bit!

Here's what I'd summarized in terms of action items (and in the mind set of convention v1):

• review the convention and echopype's handling of variuos position and offset variables (water_level, vertical_offset, transducer_position_x/y/z, MRU_position_x/y/z
• ensure compliance of required variables in v0.6.0

Seems that we should have a focused discussion about this, to clear things up.

For PR #579, I think we should merge this PR with:

1. revert back range_offset to water_level
2. keep the addition of water_level into the output Sv dataset
3. add tests for making sure water_level is corrected stored and carry it through the calibration

I am suggesting merging #579 so that we can have water_level in the Sv dataset, as part of the Sv/MVBS enhancement/convention compliance effort. I'll copy these 3 items to a comment in #579 and we can continue this part of discussion there.

[leewujung]

To clean up:

• the issue of missing offset and position variables are now captured in Add missing required variables in Platform groups #592
• the need for a test to check if water_level is corrected stored in the output of open_raw is capture in Addition of unit tests for the variable water_level #586

So we can now close this issue since the original goal (to include water_level in the Sv dataset) has been accomplished.