search

OSOceanAcoustics / echopype

Enabling interoperability and scalability in ocean sonar data analysis
https://echopype.readthedocs.io/
Apache License 2.0
95 stars 73 forks source link

Make `Platform/NMEA.time1` identical to `Platform.time1` (for EK60/EK80) #673

Closed b-reyes closed 2 years ago

b-reyes commented 2 years ago

In issue #656 we found that it is important to make Platform/NMEA.time1 identical to Platform.time1 (for EK60/EK80) and that it is best to address this in V0.6.0. This topic has been discussed in #580, however, for this issue we will not address all items discussed in #580 (as some of them can be made in the v0.6.1 release).

To make this change we need to remove the selection of certain times based off of messages in the lines below

https://github.com/OSOceanAcoustics/echopype/blob/e0b3cbff9459b57cc8320388cc1ac86da6063343/echopype/convert/set_groups_base.py#L145-L150

b-reyes commented 2 years ago

Small correction: The goal of this issue should be to make Platform.time1 identical to Platform/NMEA.time1 (for EK60/EK80) as Platform.time1 is a subset of Platform/NMEA.time1.

b-reyes commented 2 years ago

Based on an offline discussion between @leewujung and me, we found that it is best to NOT make Platform.time1 identical to Platform/NMEA.time1 (for EK60/EK80).

The reasoning behind this:

  1. By making Platform.time1 identical to Platform/NMEA.time1, this makes it so that Platform.time1 can have repeated time values (something that wasn't apparent when we first created this issue). The problem with repeated time values is that several downstream items are impacted by this.
    • For example, in EchoData.compute_range() we often use EnvParams._apply(), which eventually uses the Xarray interp function with respect to Platform.time1. However, the interp function does not allow for repeated values. Thus, we have to remove these repeated times if we want to use it.
  2. For the Platform group, the only variables that use time1 are latitude, longitude, and sentence_type. The really important variables in this group (in regards to time1) are latitude and longitude. As it currently stands, Platform.time1 only corresponds to GPS NMEA sentences. Thus, by making Platform.time1 identical to Platform/NMEA.time1 the only upside is that sentence_type can now have all of the NMEA sentence identifiers in it. The downside is that the variables latitude and longitude now need to have NaNs for NMEA sentences that are not GPS NMEA sentences and we now have repeated time1 values.
leewujung commented 2 years ago

Thanks @b-reyes for summarizing the result of our discussion. I'll close this issue now.