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

Remove NMEA sentence selection from open_raw #580

Closed
2 tasks
leewujung opened this issue Mar 11, 2022 · 6 comments
Closed
2 tasks

Remove NMEA sentence selection from open_raw #580

leewujung opened this issue Mar 11, 2022 · 6 comments
Assignees
Labels
enhancement This makes echopype better
Milestone

Comments

@leewujung
Copy link
Member

Context

This is leftover work from the overhaul at v0.5.0:

We should remove the variable NMEA_SENTENCE_DEFAULT that is defined in

NMEA_SENTENCE_DEFAULT = ["GGA", "GLL", "RMC"]

and
NMEA_SENTENCE_DEFAULT = ["GGA", "GLL", "RMC"]
(The second one is not currently used -- we should deprecate the old convert mechanism, which will be addressed in #506.)

We are pretty clear now that we want the level 0 converted data product to be authentic reproduction of the raw data generated from the instruments. Therefore, there should not be any selection of data within open_raw, so we need to remove this mechanism.

We should have this mechanism as a data processing function for higher levels, since not all NMEA sentences have the same resolution and that can cause problems in plotting ship tracks, but that would be a separate issue/PR.

Task

  • Remove NMEA_SENTENCE_DEFAULT and related sentence selection mechanism
  • Create test to ensure all sentences are stored in "/Platform/NMEA" -- one of the hake 2017 files would be good, because they have a mixture of NMEA sentence types
@leewujung leewujung added the enhancement This makes echopype better label Mar 11, 2022
@leewujung leewujung moved this to Todo in Echopype Mar 11, 2022
@leewujung leewujung added this to the 0.6.1 milestone Mar 29, 2022
@leewujung leewujung changed the title Remove NMEA sentence selection open_raw Remove NMEA sentence selection from open_raw Apr 14, 2022
@leewujung leewujung modified the milestones: 0.6.1, 0.6.2 Jun 15, 2022
@b-reyes
Copy link
Contributor

b-reyes commented Jul 28, 2022

@leewujung please correct me if I am misunderstanding something, but based off of #673 (comment) it seems like we should not implement this (at least for the EchoData object). As I recall, it will cause several downstream issues that have little to no upside.

We are pretty clear now that we want the level 0 converted data product to be authentic reproduction of the raw data generated from the instruments.

If our goal is to have level 0 be an authentic reproduction of the raw data, then does it make sense to make open_raw have an optional argument that would instead return a pandas dataframe that is just the parsed data?

@leewujung
Copy link
Member Author

leewujung commented Jul 28, 2022

@b-reyes : I am not sure what you mean, because this PR is exactly to remove the currently unused (and cannot be used) NMEA sentence selection mechanism, so that there is no optional argument in open_raw to do any selection.

@leewujung
Copy link
Member Author

@b-reyes : reading what i wrote earlier again, I guess we were probably talking about different things... Maybe you were referring to the fact that we cannot preserve all the NMEA sentences? If so, I agree.

I think my issue cobbled two things so probably can be revised. In my mind the thing to be removed is the option for user to say they only want a further subset of those GPS position sentences. (and that we still have to select these sentences out from all the NMEA sentences).

@b-reyes
Copy link
Contributor

b-reyes commented Jul 28, 2022

I think my issue cobbled two things so probably can be revised. In my mind the thing to be removed is the option for user to say they only want a further subset of those GPS position sentences. (and that we still have to select these sentences out from all the NMEA sentences).

@leewujung that makes sense now. Great, I have no problem with this.

Does this new objective sound good @leewujung?

Objective: Remove the option that allows the user to select a subset of the GPS position sentences and we should continue to select the same NMEA sentences. Specifically, we should only select the NMEA sentences: ["GGA", "GLL", "RMC"] .

@leewujung
Copy link
Member Author

@b-reyes : Yes, I think you are spot on! Thanks for keeping clear eyes on this! Feel free to edit the issue text as you see fit.

@b-reyes
Copy link
Contributor

b-reyes commented Aug 11, 2022

PR #778 achieved the new objective of this issue, I will go ahead and close this now.

@b-reyes b-reyes closed this as completed Aug 11, 2022
Repository owner moved this from Todo to Done in Echopype Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This makes echopype better
Projects
Status: Done
Development

No branches or pull requests

2 participants