Add additional EK80 environment variables

[imranmaj]

Includes additional EK80 environment variables in the converted dataset.

Resolves #540

[imranmaj]

@leewujung I didn't include the latitude field in the raw file which (according to the discussion in #540) is used to calculate sound speed; should that be included also? I wasn't sure if it was necessary since the latitude is already in the Platform group and also the sound_speed, which I assume uses the latitude, has already been calculated and included as sound_speed_indicative, so there isn't any use for it anymore.

[codecov-commenter]

Codecov Report

Merging #616 (1fe51f5) into dev (4bee4fa) will decrease coverage by 5.36%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #616      +/-   ##
==========================================
- Coverage   78.70%   73.34%   -5.37%     
==========================================
  Files          42       15      -27     
  Lines        3776     2326    -1450     
==========================================
- Hits         2972     1706    -1266     
+ Misses        804      620     -184     

Flag	Coverage Δ
unittests	73.34% <100.00%> (-5.37%)	⬇️

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

Impacted Files	Coverage Δ
echopype/convert/set_groups_ek60.py	91.58% <ø> (ø)
echopype/convert/set_groups_ek80.py	96.40% <100.00%> (+0.15%)	⬆️
echopype/convert/set_groups_base.py	88.17% <0.00%> (-2.16%)	⬇️
echopype/convert/utils/ek_raw_parsers.py	54.90% <0.00%> (-0.17%)	⬇️
echopype/echodata/convention/utils.py
echopype/preprocess/api.py
echopype/echodata/echodata.py
echopype/echodata/widgets/utils.py
echopype/testing.py
echopype/calibrate/ecs_parser.py
... and 21 more

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

[leewujung]

@leewujung will look into why there is no depth specification attached to the profile.

[leewujung]

#540 (comment) and the following comments clarified this.

@imranmaj : Once we settle down the variable name and attributes re #540 (comment) and #540 (comment) please add those to this PR. Thanks!

[leewujung]

@emiliom : here do you think we should keep the original names transducer_* instead of changing to sonar_*? Since it is not entirely clear what this is other than my conjectured use of line of sight time delay to measure sound speed, it seems better to keep its original name. This is not in the convention.

[gavinmacaulay]

The EK80 has a transducer_sound_speed value separate from water column sound speed because some of their systems potentially do a correction for transducer parameters based on the sound speed at the transducer face (e.g., beamwidth changes with sound speed across the transducer face).

[leewujung]

Interesting... Thanks @gavinmacaulay for explaining where this is from!
Is this something that's done automatically based on the transducers? or something the users can toggle on/off?

[gavinmacaulay]

I've never actually seen a Simrad system that uses this, but think that the bathymetric systems may have a use for it.

[leewujung]

@imranmaj : As we discussed last week and I have specified in #540 (comment), all the environmental variables have a time dimension that come with the Environment datagram -- it is NOT ping_time for EK80 because the data are not part of the Raw datagram (which has the acoustic data). Please add a time dimension.

@emiliom : Need your inputs here:
Q1: For the variables that are saved into the Platform group, I am thinking we can use time3 so that the current location_time and mru_time can become time1 and time2 (#518). What do you think?

Q2: For the variables that are saved in the Environment group, should we use environment_time or time3? The latter would make it consistent across groups.

Q3: What do you think about adding comments or long_name on where the timestamps come from?

[leewujung]

Thanks @imranmaj ! All the changes look great. I was going to approve it but realized that I forgot to ask for tests... could you please add tests to these also just like in #631? Just so that we have our base covered for the v0.6.0 endeaver.

[emiliom]

Q1: For the variables that are saved into the Platform group, I am thinking we can use time3 so that the current location_time and mru_time can become time1 and time2 (#518). What do you think?

Yeah, time3 makes sense, since it's the third time dimension used.

Q2: For the variables that are saved in the Environment group, should we use environment_time or time3? The latter would make it consistent across groups.

IMHO it's fine but somewhat weird to use time3 in the Environment group. The 3 only makes sense within the Platform context. I think I'd lean towards something more specific like environment_time or something closer to the instrument type. But, frankly, it's a toss up! Consistency with Platform is an advantage, too.

Q3: What do you think about adding comment or long_name on where the timestamps come from?

A long_name attribute should ALWAYS be included in general, and ALWAYS^10 for coordinate variables. I've added inline comments to that effect. The same long_name string should be used in both Platform and Environment, if they're really the same time sources/arrays.

I suggest adding a comment attribute that looks something like this:

"comment": "Platform.time3 and Environment.environment_time are identical time coordinates from the same sensor"

[leewujung]

Thanks @emiliom !

@imranmaj : please use the following:

• time3 in the Platform group
• environment_time in the Environment group
• in both groups: attributes for time3 and environment_time

  {
      "axis": "T",
      "long_name": "Timestamps for Environment XML datagrams",
      "standard_name": "time",
      "comment": "Platform.time3 and Environment.environment_time are identical time coordinates from the same datagrams",
  },
  
  

Note that I changed the end of the comment attribute from "the same sensors" to "the same datagrams" to be more general.

[leewujung]

@imranmaj : two comments:

• Since the blocks you have are in the same file and identical, how about consolidating them to a function?
• You are currently testing for the existence but not the values. Could you add tests for values too?

[imranmaj]

• Since the blocks you have are in the same file and identical, how about consolidating them to a function?

Is this referring to the tests? Is there a way to add functions to test files without running them as tests? I’ve mostly found answers like this: https://stackoverflow.com/questions/53063852/pytest-how-do-you-test-a-function-and-not-run-the-rest-of-the-file

[leewujung]

Oh I wasn't actually thinking that much, I though we already did something like this in

echopype/echopype/tests/preprocess/test_preprocess.py

Line 201 in a9c19f7

def _construct_MVBS_test_data(nfreq, npings, nrange_samples):