AD2CP Data Format Reorganization

[imranmaj]

Reorganizes the AD2CP data format as was done with other formats for v0.6.0

Runs with datatree v0.0.4 on non-Windows machines (see #732)

Todo:

• Move some variables from beam to vendor
• Move echosounder_raw variables from vendor to beam
  • Rename echosounder_raw variables
• Change pulse compression to a vector
• Separate beam group into multiple groups
• Rename sample to range_sample and sample_transmit to transmit_sample
• Rename range_sample_X's to just range_sample within their own beam group
• Rename combined ping_time to time1
• Rename individual ping_time_Xs to just ping_time within their own beam group
• Move magnetometer_raw from platform to vendor
• Add descr for each beam group
• Add long_name, comment, and units for every variable
• Update tests

Resolves #711, #733

See also: #719, link to reorganization details

[imranmaj]

It looks like the EchoData repr only expects 2 beam groups right now so it's erroring with rawtest.076.00000.ad2cp which has 3 (burst, echosounder, and echosounder raw)

[leewujung]

It looks like the EchoData repr only expects 2 beam groups right now so it's erroring with rawtest.076.00000.ad2cp which has 3 (burst, echosounder, and echosounder raw)

I think we can have up to 4 for AD2CP. Tagging @lsetiawan here: could we change it to have arbitrary number of Beam_groupX?

[lsetiawan]

could we change it to have arbitrary number of Beam_groupX?

Probably, but right now I think we are still limited because of the convention yaml! I think this has to change. In order to do that I think we need to really pull away from the .group access and also not use convention yaml for repr. I'll need to look into this further. Should I be making changes in this PR for this or separate?

[codecov-commenter]

Codecov Report

❌ Patch coverage is 98.94737% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.07%. Comparing base (eaeb590) to head (0262462).
⚠️ Report is 424 commits behind head on dev.

Files with missing lines	Patch %	Lines
echopype/convert/set_groups_ad2cp.py	98.36%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #731      +/-   ##
==========================================
- Coverage   81.89%   73.07%   -8.82%     
==========================================
  Files          45       19      -26     
  Lines        4137     2626    -1511     
==========================================
- Hits         3388     1919    -1469     
+ Misses        749      707      -42     

Flag	Coverage Δ
unittests	73.07% <98.94%> (-8.82%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[imranmaj]

@leewujung finally got the tests to pass so this should be ready to go

[leewujung]

@imranmaj : thanks! I'll look into this tomorrow morning. I noticed that the requirements are not updated -- I thought updating datatree to 0.0.6 is needed for you to run tests on your machine? #732 (We do need users to be able to run on Windows, so we probably need to pin that version and continue to update as datatree develops.)

[imranmaj]

I was able to run the tests by manually editing the datatree library source code on my machine

[imranmaj]

@imranmaj : I meant to check the actual values of the number of beams, physical beam used etc in the Beam_groupX groups. I'll do that when the time1 vs ping_time1 issue is resolved, since it is a bit messy to checked those now.

@leewujung All of the above issues, including that one, should be fixed now

[leewujung]

@imranmaj : Thanks! I'll check in a couple hours on the changes.

[leewujung]

This was embedded so highlighting it here since this variable is still in the beam groups from the latest parsed files.

Also, what is the velocity_range variable in this? I couldn't find correspondence in the data spec, but it exists only in your BURST_AVERAGE_VERSION2_DATA_RECORD_FORMAT format and not in version 3.`

[leewujung]

The data_set_description seems to encode different things for different beam groups. It seems that one is parsed and the other is unparsed. I think for Sonar_group1 and Sonar_group2 that's because you use the parsed values for the beam coordinate -- is this true? If so we need to do something to describe this difference of behavior.

Below again from rawtest.090.00001.nc
Sonar_group1 and Sonar_group2 look like this

Sonar_group3 and Sonar_group4 look like this:

[imranmaj]

I think for Sonar_group1 and Sonar_group2 that's because you use the parsed values for the beam coordinate -- is this true?

Yes, the beam coordinate comes from data_set_description

[imranmaj]

what is the velocity_range variable in this?

All the references to it in the spec pretty much just say this:

[leewujung]

For velocity_range, what I meant was that in the data spec it only exists for BURST_AVERAGE_VERSION2_DATA_RECORD_FORMAT and not BURST_AVERAGE_VERSION3_DATA_RECORD_FORMAT, but I believe the data we use for testing are all from version 3, so what does velocity_range exist in the converted data?

[leewujung]

For echosounder_raw_echogram:
Could you remind what the logic below (setting echosounder_raw_echogram in the previous packet to be the current index) is? If you make this simply using the current ping's data, does it work?

echopype/echopype/convert/parse_ad2cp.py

Lines 742 to 748 in efbf983

	if (
	self.parser.packets[-1].is_echosounder_raw()
	or self.parser.packets[-1].is_echosounder_raw_transmit()
	):
	self.parser.packets[-1].data["echosounder_raw_echogram"] = self.data[
	"echosounder_index"
	]

[imranmaj]

If you make this simply using the current ping's data, does it work?

No, echosounder_index does not exist in the echosounder raw data record format

[imranmaj]

so why does velocity_range exist in the converted data?

Currently all variables from all formats are always included regardless of whether they exist, I think because we decided that it would be confusing for users when variables are sometimes included and sometimes not included

[leewujung]

All tests passed in this major reorganization work, so I'll merge this now. Leftover work captured in #732, #734, and saving config parameters in Vendor_specific group as discussed in #719 (comment).