Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
b-reyes opened this issue May 11, 2022 · 3 comments
Closed
Assignees
Labels
data conversion enhancement This makes echopype better
Milestone

Comments

@b-reyes
Copy link
Contributor

b-reyes commented May 11, 2022

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

def _parse_NMEA(self):
"""Get the lat and lon values from the raw nmea data"""
messages = [string[3:6] for string in self.parser_obj.nmea["nmea_string"]]
idx_loc = np.argwhere(np.isin(messages, self.ui_param["nmea_gps_sentence"])).squeeze()
if idx_loc.size == 1: # in case of only 1 matching message
idx_loc = np.expand_dims(idx_loc, axis=0)

@b-reyes b-reyes added this to the 0.6.0 milestone May 11, 2022
@b-reyes b-reyes self-assigned this May 11, 2022
@leewujung leewujung moved this to Todo in Echopype May 11, 2022
@leewujung leewujung added enhancement This makes echopype better data conversion labels May 11, 2022
@b-reyes
Copy link
Contributor Author

b-reyes commented May 17, 2022

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
Copy link
Contributor Author

b-reyes commented May 18, 2022

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
Copy link
Member

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

Repository owner moved this from Todo to Done in Echopype May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data conversion enhancement This makes echopype better
Projects
Status: Done
Development

No branches or pull requests

2 participants