AZFP platform group will now include vertical_offset data by calculating depth

[praneethratna]

Addresses 2nd step of #1181 (comment), by including calculations for depth value mentioned in #1181 (comment) which is used to set the vertical_offset variable.

CC @emiliom

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3335cae) 83.27% compared to head (8ada074) 77.60%.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1202      +/-   ##
==========================================
- Coverage   83.27%   77.60%   -5.67%     
==========================================
  Files          64       16      -48     
  Lines        5668     2630    -3038     
==========================================
- Hits         4720     2041    -2679     
+ Misses        948      589     -359     

Flag	Coverage Δ
unittests	77.60% <100.00%> (-5.67%)	⬇️

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.

[praneethratna]

@emiliom I have raised a separate PR #1207 for addition of depth_from_pressure method along with tests for that method as suggested by you. Once that is merged, I'll re-work on the commits here to include the changes only for setting vertical_offset value in this PR. Thanks!

[emiliom]

There are some open questions that I've raised, some of which were on me to follow up on. I'm compiling and summarizing all of them here, with some updates:

1. Is atmospheric pressure (and potentially other overlying pressure effects) typically already removed from pressure measurements?
   • It's not unusual to remove atmospheric pressure in the sensor calibration, so that pressure = 0 represents the surface. We've accounted for this in PR Added depth_from_pressure method required for the calculation of vertical_offset value #1207 in the eneric depth_from_pressure function by setting the default value of the atm_pres_surf to zero.
   • Now the question is whether an AZFP pressure sensor is hard wired to always remove atmospheric pressure, to never remove it, or if it's a user configuration. That will determine whether to pass atm_pres_surf a value of 0 or 10.1325 (dbar) in the call to depth_from_pressure, or whether it's undetermined.
2. Assigning a value to water_level
   • For AZFP, we're allowing water_level to only be either np.nan or 0.0.
   • If pressure values are not all np.nan (there are valid values), water_level should be set to 0.0. Otherwise, if pressure is all np.nan, water_level will be np.nan
3. Based on the definitions in the echopype documentation, the platform coordinate system origin corresponding to the position of the pressure sensor, and water_level being set to zero, the assignment vertical_offset = -pressure_sensor_depth is correct, and vertical_offset will be negative.
4. The echosounder pointing direction (up or down) doesn't impact the calculations and assumptions used here for setting vertical_offset and water_level.
5. Time dimension for the pressure variable, and therefore for the assigned vertical_offset variable
   • If tilt_x and tilt_y have valid values, they will already be using a time2 dimension. Do we know if pressure actually shares the same clock, so we can reuse that dimension?
   • If tilt_x and tilt_y are all np.nan, they're pre-assigned with length-1 time2 dimension. pitch and roll will also use this dummy, "place holder" dimension. Should pressure be assigned a new dimension, time3? Or should its dimension be called time2, then tilt_x, tilt_y, pitch and roll will become
     arrays of np.nan with length >1?
6. Test for unpacked_data["pressure"] == np.nan. If True, skip the the call to depth_from_pressure.

[emiliom]

Regarding point 5 above: I've confirmed in parse_azfp.py (see the parse_raw method) that pressure, when present, will be on the same "clock" as tilt_x and tilt_y. So, when present and used to generate vertical_offset, vertical_offset can reuse the time2 time dimension used by tilt_x and tilt_y. If tilt_x and tilt_y have valid data, their length and time values will be identical to those of vertical_offset. If tilt_x and tilt_y are all nan and set to be of length 1, I guess we'll just extend them to the full time dimension of vertical_offset.

[praneethratna]

@emiliom I have pushed the changes which addresses most of the points mentioned except one where we are still not sure whether to pass atm_surf_pres a value of 0 or 10.1325 (dbar).

[emiliom]

I have pushed the changes which addresses most of the points mentioned

Thanks! I'll look over your commits today.

except one where we are still not sure whether to pass atm_surf_pres a value of 0 or 10.1325 (dbar).

We haven't heard back from Steve on that question (I believe you were cc'd on that email). I'll follow up again today.

Also, I just realized that I forgot to mention here a related PR I submitted, #1226. If approved, the main impact on this PR is that the pressure variable will be created in set_groups_azfp.py only when the AZFP data file actually contains pressure data. Right now it's always created and it's filled with np.nan when the data file doesn't have pressure data.

Finally, @leewujung made a comment related to this PR (#1222 (comment)):

I am a bit hesitate regarding converting pressure to depth directly, because of the uncertainty in latitude. I'll comment on it in #1202

[emiliom]

except one where we are still not sure whether to pass atm_surf_pres a value of 0 or 10.1325 (dbar).

We heard back from Steve about this! In summary, it sounds like their pressure measurements are almost always, if not always, uncorrected ("uncompensated") for atmospheric pressure. You already apply this atmospheric correction in parse_azfp.py::_compute_pressure, so using atm_surf_pres a value=0 in the call to depth_from_pressure is correct. However, the value you used in _compute_pressure is the one from the AZFP Matlab code, 10.125. Let's use the officially accepted value instead, 10.1325 dbar.

[emiliom]

Thanks @praneethratna ! All your changes look good.

[leewujung]

If I understand the various emails and comments surrounding this PR correctly, I don’t think we should add vertical_offset based on depth calculated by pressure sensor sensor reading by assigning latitude=30. IMHO we should leave the user only with the pressure sensor values, so they can decide what they want to do to convert it to depth or vertical_offset. I think adding a vertical_offset value automatically would likely be misleading, especially if people do not carefully consider where the value comes from.

Since the pressure to depth function already exists (thank you!), we can add to the docs to let users know that this function exists if they want to convert the pressure reading to depth by themselves.

On the time stamp: tilt_x/y and pressure and many other sensor readings are read from the same time base, because they are wrapped in the same data packet.

[emiliom]

That's reasonable. That means we won't merge this PR, since the updating of vertical_offset (and the related water_level) was the sole focus of the PR. I suggest we leave the PR open for the time being, though, in case we want to revisit the issue in the future. In the meantime, I'll change it to a "Draft".

As reference, it's worth noting that in the AZFP Matlab toolbox, the computeDepth function in LoadAZFP.m also doesn't account for latitude -- for the same reason, since latitude normally would not be available.

Since the pressure to depth function already exists (thank you!), we can add to the docs to let users know that this function exists if they want to convert the pressure reading to depth by themselves.

Sounds good!

[leewujung]

I'll close this PR now since adding vertical_offset based on the pressure value could be misleading without knowing what the actual latitude value is.