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

Added parser support for EK80 MRU1 #1242

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

praneethratna
Copy link
Contributor

Addresses #1239 and now EK80 files of type MRU version 1 can be parsed.

CC @leewujung

@praneethratna praneethratna self-assigned this Dec 10, 2023
@praneethratna praneethratna added the enhancement This makes echopype better label Dec 10, 2023
@praneethratna praneethratna added this to the 0.8.3 milestone Dec 10, 2023
Copy link
Member

@leewujung leewujung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@praneethratna : Thanks for putting this in! I just have a minor comment about adding MRU1 in the docstring. I verified that the rest of open_raw runs through fine with the pitch, roll, heave data stored in the Platform group as pitch, roll, and vertical_offset.

But the parsed heading variable is not stored. Let me take a look at the convention to see if there's a variable we can store it, and if not, we can put it in the Platform group just with heading as the name. I will get back to you on this!

It'll be nice to have a test file in the CI for this.

@jmjech: Do you have a file that is smaller (say a few MB) that we can use as a test file? It adds to the time for tests to run through if the test files are large, so we usually try to include only small files.

Thanks both!

@jmjech
Copy link

jmjech commented Dec 14, 2023

I e-mailed a 2 MB file to Wu-Jung. Let me know if it works for you. I have other "smaller" files.
For some reason it is not allowing me to add the data files here.

@leewujung
Copy link
Member

leewujung commented Dec 14, 2023

@jmjech: Thank you! The file works and does contain MRU1 datagrams.

@praneethratna: I have added this file (20231016_Cal_-D20231016-T220322.raw) under test_data/ek80.
Could you please add a test for parsing it? In looking into this file I also checked for:

np.all(echodata["Platform"]["pitch"].data == np.array(parser.mru["pitch"]))
np.all(echodata["Platform"]["roll"].data == np.array(parser.mru["roll"]))
np.all(echodata["Platform"]["vertical_offset"].data == np.array(parser.mru["heave"]))  # note variable name change

Would be good to have these in the tests, by using the parser object

parser.parse_raw()

Thanks!!

@leewujung
Copy link
Member

leewujung commented Dec 14, 2023

Also, on storing the heading variable. It is indeed in the ver.1 convention as below:

Screenshot 2023-12-14 at 11 43 23 AM

@praneethratna: Could you please add this to the set group so that it gets stored together with the other variables? As you have seen, they are on the same timebase since they are all from the MRU* datagrams. Make sure to add the attributes listed. Thanks!

@praneethratna
Copy link
Contributor Author

praneethratna commented Dec 18, 2023

@leewujung I have added docstring for new MRU1 type datagram and also added a new test case for the same using the file given by @jmjech. Also, now the heading variable will be stored in Platform group with the attributes that you've mentioned. Thanks!

Copy link
Member

@leewujung leewujung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@praneethratna : Thanks for adding heading and test. I'll merge this now. :)

@leewujung leewujung merged commit 529fa60 into OSOceanAcoustics:dev Dec 18, 2023
5 checks passed
@jmjech
Copy link

jmjech commented Dec 19, 2023

Thanks!
I will test the code on my computer and let you know if I have any issues.

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

Successfully merging this pull request may close these issues.

3 participants