AZFP ULS6 beta version, ULS5 fixes#1581
AZFP ULS6 beta version, ULS5 fixes#1581dash-uvic wants to merge 8 commits intoOSOceanAcoustics:mainfrom
Conversation
…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
|
I haven't gotten around to upload the test data to GitHub release assets just yet. Traveling tomorrow, so will try Wednesday... |
|
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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1581 +/- ##
==========================================
+ Coverage 84.95% 85.27% +0.32%
==========================================
Files 79 80 +1
Lines 6998 7042 +44
==========================================
+ Hits 5945 6005 +60
+ Misses 1053 1037 -16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@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
left a comment
There was a problem hiding this comment.
@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.pyand will ALWAYS executeparse.parse_raw()for all sonar models.As you make changes re the parent/child classes, could you also remove these?if not self.unpacked_data: self.parse_raw()
|
@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. |
|
@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
|
Hi @dash-uvic, the error currently raised in the tests is fixed in #1591, so fetching the latest from upstream should resolve it! |
|
@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. |
|
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. |
ULS5 Bug fixes:
ULS6 updates: