AZFP ULS6 beta version, ULS5 fixes by dash-uvic · Pull Request #1581 · echostack-org/echopype

dash-uvic · 2025-12-02T16:47:21Z

ULS5 Bug fixes:

fixed recursive bug for corrupted files
fixed RepeatedField not applying correctly
ULS6 updates:
Added AZFP Nano (single channel) compatibility
Added compatibility for beta version of ULS6 standard
Refractored code to inherit ULS5 as a base class
Added additional tests for ULS6 to include Paros sensors

…earlier .azfp extensions. Fixed RepeatedField bug and recursive bug loading corrupted files. Added support for single-channel AZFP models. Added additional tests for AZFP6 and latest .asp6 data files

leewujung · 2025-12-02T22:18:11Z

I haven't gotten around to upload the test data to GitHub release assets just yet. Traveling tomorrow, so will try Wednesday...

leewujung · 2025-12-06T15:28:16Z

Hey @dash-uvic : I got the test data you sent me as a staged asset in #1582. It’s merged so if you pull from main now you should have all the right files.

codecov-commenter · 2025-12-06T19:45:01Z

Codecov Report

❌ Patch coverage is 94.03748% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.06%. Comparing base (6cf6cee) to head (6a7519b).
⚠️ Report is 37 commits behind head on main.

Files with missing lines	Patch %	Lines
echopype/convert/parse_uls6.py	89.43%	26 Missing ⚠️
echopype/convert/parse_azfp.py	92.06%	5 Missing ⚠️
echopype/convert/parse_uls5.py	97.22%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1581      +/-   ##
==========================================
+ Coverage   85.58%   86.06%   +0.48%     
==========================================
  Files          79       79              
  Lines        6998     7084      +86     
==========================================
+ Hits         5989     6097     +108     
+ Misses       1009      987      -22

Flag	Coverage Δ
integration	`80.98% <89.94%> (+0.34%)`	⬆️
unit	`61.09% <52.29%> (+0.67%)`	⬆️
unittests	`85.96% <94.03%> (+0.48%)`	⬆️

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.

leewujung · 2025-12-07T15:53:46Z

@dash-uvic: I pulled the update assets in and the current tests all passed. Are there additional tests or code you want to add?

leewujung

@dash-uvic : Thanks for the PR! I went through the changes quickly, and have one more substantial suggestion and a couple small ones below.

I think it'd be better from the maintenance point of view to have a parent class parse_AZFP and have 2 child classes, one parse_ULS5 and the other parse_ULS6. What you have under the current parse_AZFP (largely from existing code) can largely stay in this parent class, with both child classes being pretty slim with only the differences on the header and XLM handling.

Small comments:

I don't see this constant used in the ULS6 code: FILENAME_DATETIME_AZFP = "\\w+_\\w+.[azfp|aps6]". Could you confirm? This is linked to the parent class suggestion above. It'll be cleaner to have all shared constants in the parent class and only the differences showing up in the child class. We did the same thing for the EK classes.
There are these couple lines in multiple functions that I think are redundant, since the parse object is called from convert/api.py and will ALWAYS execute parse.parse_raw() for all sonar models.
```
if not self.unpacked_data:
  self.parse_raw()
```
As you make changes re the parent/child classes, could you also remove these?

dash-uvic · 2025-12-17T23:12:23Z

@leewujung I've done the suggested changes and included comments from [1505] and moved the Sv_offset to calibration. Just need to write a test for the new Sv interpolation and I'll push.

leewujung · 2025-12-18T18:18:59Z

@dash-uvic : Awesome. Looking forward to it!

…alibrate and added calibration tests. Now Sv_offset attempts interpolation for non-defined Freq/PulseLen and returns 0.0 with a warning otherwise

LOCEANlloydizard · 2025-12-27T10:52:32Z

Hi @dash-uvic, the error currently raised in the tests is fixed in #1591, so fetching the latest from upstream should resolve it!
Cheers!

leewujung · 2025-12-31T23:39:06Z

@dash-uvic: I pulled in the upstream and everything runs through fine, so you should be good to go on adding additional tests for Sv offset.

dash-uvic · 2026-01-09T17:56:32Z

Sorry just got back from vacation. I just did a pull and ran the tests and everything looks good (Sv offset tests were in the last push). I did a minor code cleanup push just now to remove a few commented out asserts.

leewujung · 2026-06-06T18:14:11Z

@dash-uvic : I have asked @LOCEANlloydizard to help review this PR, so that I don't continue to be the bottleneck. 😵

@LOCEANlloydizard : there are some discussions in #1505 that @dash-uvic has included in this PR, in addition to refactoring the AZFP ULS data classes. Thanks again for taking this on!

Co-authored-by: Wu-Jung Lee <leewujung@gmail.com>

LOCEANlloydizard

Hi @dash-uvic, thanks for the PR! I merged the small conflict, but I don't have much more to add beyond the inline comments! Overall this looks good to me and address the earlier discussion around moving to a shared AZFP base class with ULS5/ULS6 implementations! The test coverage also looks good, with the new integration tests against files and Matlab outputs (don't know if we want some unit tests?)

I was wondering about the Sv_offset interpolation: from the discussion in #1505 I understood the main goal as handling pulse lengths that were not explicitly listed in the lookup table (and possibly extrapolating ?). The examples discussed there seemed to propose 1D interpolation over pulse length, whereas the implementation use a 2D interpolation across both frequency and pulse length, with no extrapolation outside the interpolation grid? just to make sure!

Other than that my comments are mostly small cleanup questions, and once those are clarified this looks good to me! Cheers!

LOCEANlloydizard · 2026-06-12T03:57:48Z



-@pytest.mark.skip(reason="required pulse length not in Sv offset dictionary")
+#@pytest.mark.skip(reason="required pulse length not in Sv offset dictionary")


should we remove the line and not comment?

Yeah we can remove the line as its just a leftover from using the old Sv offset dictionary.

LOCEANlloydizard · 2026-06-12T04:01:57Z

+    # Check if outside of calibration grid
+    if np.isnan(zq):
+        raise ValueError(
+            f"Pulse lengths less than 150 or greater than 1000 usecs for {frequency}kHz "


should be in Hz and not kHz?

Yes your right that should be Hz.

LOCEANlloydizard · 2026-06-12T04:03:51Z

+                except ValueError:
+                    logger.warning(
+                        f"The Sv for {freq}kHz and pulse length {pulse_len}us "
+                        "is uncalibrated (Sv_offset=0.0)"


units ok too?

Should be Hz as well.

LOCEANlloydizard · 2026-06-12T04:27:29Z

+            self.unpacked_data["data_type"][ping_num] = [self.unpacked_data["data_type"][ping_num]]
+            self.unpacked_data["range_samples_per_bin"][ping_num] = [
+                self.unpacked_data["range_samples_per_bin"][ping_num]
+            ]


would it make sense to normalize these fields in _split_header() when num_chan == 1?

I stuck it in there since _split_header is different between parse_uls5/parse_ulse6 and didn't make sense to duplicate code there twice. But if it makes more sense in the _split_header we could put it there, and if would make sense if the ULS6 spec ever changed its behaviour with single frequency AZFPs.

LOCEANlloydizard · 2026-06-12T04:33:30Z

+        sonar_model="AZFP",
+        **kwargs,
+    ):
+        super().__init__(file, storage_options, storage_options, sonar_model)


replace first occurrence with file_meta

LOCEANlloydizard · 2026-06-12T05:01:01Z

-    assert np.allclose(
-        ds_matlab_output['Output']['BatteryTx'][0][0].squeeze(),
-        echodata["Vendor_specific"].battery_tx,
-        rtol=1e-08


we remove test for battery?

nevermind, saw it in test_convert_azfp_battery

dash-uvic · 2026-06-15T15:03:15Z

@LOCEANlloydizard I discussed it with our in-house AZFP expert since acoustics are not my area and this seemed to be an reasonable approach. Outside the interpolation grid the AZFP/transducers don't operate properly anyway. Do i need to make these edits or do you plan to? I can get them done today if I do.

LOCEANlloydizard · 2026-06-15T17:40:14Z

Hi @dash-uvic, ok nice thank you! Yes, if you don't mind making the changes after pulling the updated version (following the merge, etc.), then we can take a look from there! Thanks again!

…1 to _split_header in uls5/uls6

asldash and others added 2 commits November 21, 2025 11:58

Added support for latest ULS6 aps6, with backward compatability with …

e3ec4f5

…earlier .azfp extensions. Fixed RepeatedField bug and recursive bug loading corrupted files. Added support for single-channel AZFP models. Added additional tests for AZFP6 and latest .asp6 data files

Merge branch 'OSOceanAcoustics:main' into main

0e646db

leewujung mentioned this pull request Dec 6, 2025

Update azfp6 test data #1582

Merged

leewujung closed this Dec 6, 2025

leewujung reopened this Dec 6, 2025

Merge branch 'main-upstream'

0290349

leewujung requested changes Dec 8, 2025

View reviewed changes

dash-uvic added 2 commits December 11, 2025 11:31

Merge branch 'OSOceanAcoustics:main' into main

0e60086

Merge branch 'OSOceanAcoustics:main' into main

db19caf

Renamed AZFP to ULS5/6 child classes, moved Sv_offset from parse to c…

0246938

…alibrate and added calibration tests. Now Sv_offset attempts interpolation for non-defined Freq/PulseLen and returns 0.0 with a warning otherwise

Merge branch 'main-upstream' into main-dash-uvic

596d471

Removed a couple of unused test asserts for calibrate_azfp

e1ab28e

Merge branch 'main-upstream' into main-dash-uvic

6a7519b

leewujung reviewed Apr 20, 2026

View reviewed changes

Comment thread echopype/calibrate/cal_params.py Outdated

leewujung requested a review from LOCEANlloydizard June 6, 2026 18:11

LOCEANlloydizard and others added 2 commits June 11, 2026 16:30

Merge branch 'main' into main

bd8d709

Apply suggestions from code review

eafd7e1

Co-authored-by: Wu-Jung Lee <leewujung@gmail.com>

LOCEANlloydizard reviewed Jun 12, 2026

View reviewed changes

Addressed reviewer comments, moved the field normalization when chan=…

76fac03

…1 to _split_header in uls5/uls6



		@pytest.mark.skip(reason="required pulse length not in Sv offset dictionary")
		#@pytest.mark.skip(reason="required pulse length not in Sv offset dictionary")

Conversation

dash-uvic commented Dec 2, 2025

Uh oh!

leewujung commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leewujung commented Dec 6, 2025

Uh oh!

codecov-commenter commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

leewujung commented Dec 7, 2025

Uh oh!

leewujung left a comment

Choose a reason for hiding this comment

Uh oh!

dash-uvic commented Dec 17, 2025

Uh oh!

leewujung commented Dec 18, 2025

Uh oh!

LOCEANlloydizard commented Dec 27, 2025

Uh oh!

leewujung commented Dec 31, 2025

Uh oh!

dash-uvic commented Jan 9, 2026

Uh oh!

Uh oh!

leewujung commented Jun 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LOCEANlloydizard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dash-uvic commented Jun 15, 2026

Uh oh!

LOCEANlloydizard commented Jun 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

leewujung commented Dec 2, 2025 •

edited

Loading

codecov-commenter commented Dec 6, 2025 •

edited

Loading

leewujung commented Jun 6, 2026 •

edited

Loading