Closed praneethratna closed 6 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
3335cae
) 83.27% compared to head (8ada074
) 77.60%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@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!
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:
depth_from_pressure
function by setting the default value of the atm_pres_surf
to zero.atm_pres_surf
a value of 0 or 10.1325 (dbar) in the call to depth_from_pressure
, or whether it's undetermined.water_level
water_level
to only be either np.nan
or 0.0
.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
water_level
being set to zero, the assignment vertical_offset = -pressure_sensor_depth
is correct, and vertical_offset
will be negative.vertical_offset
and water_level
.vertical_offset
variable
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?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?unpacked_data["pressure"] == np.nan
. If True
, skip the the call to depth_from_pressure
.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
.
@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).
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 (https://github.com/OSOceanAcoustics/echopype/pull/1222#discussion_r1394848585):
I am a bit hesitate regarding converting pressure to depth directly, because of the uncertainty in latitude. I'll comment on it in https://github.com/OSOceanAcoustics/echopype/pull/1202
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.
Thanks @praneethratna ! All your changes look good.
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.
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!
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.
Addresses 2nd step of https://github.com/OSOceanAcoustics/echopype/issues/1181#issuecomment-1763217291, by including calculations for
depth
value mentioned in https://github.com/OSOceanAcoustics/echopype/issues/1181#issuecomment-1781696782 which is used to set thevertical_offset
variable.CC @emiliom