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

Nortek adcp4beams updates #778

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

BecCowley
Copy link
Contributor

Changes made by Mark Snell to enable 4-beam Nortek Signature Parsing and bin mapping.
NOTE THAT THE UPDATES WILL REQUIRE MORE WORK AND TESTING PRIOR TO MERGING.

@ocehugo @sspagnol @evacougnon
Mark's notes are in the attached word document.
beam_compare.docx
Please contact Mark if you require more information or files for testing.

Notes from Hugo:
In summary, the new code is just changing some matrices indexes to account for the 4-beam (pretty much ~10 lines - 180-190 in beam2enu) but doing the calculation/access in different index/namespace. Hence the proposed changes are far from ideal since they are changing too much for too little - there should be only one way to perform the loops/access the index/incur the matmul. The conversion matrix being shaped/accessed differently for the two cases is jarringly confusing. The code has a lot of duplicated boilerplate between 3/4 beams. Hence, the code needs to be aligned and consistent between the cases handled.

Some extras will likely be needed:

1.The raw instrument file (Beam coordinates), the processed ones with the Nortek software, and references to the manual (page/equation/etc) and the mat files.
2.Tests for the transformation.
3.Tests for the binMapping of the different orientations.

I think Parser/Sig4beam_transform.m should be translated to a test (Pass/Fail) - you may use the function isequal_tol to verify the double-precision arguments up to whatever decimal precison you want and make the plots optional. This way, the code can fearlessly refactor all the code implemented. Unfortunately, the original beam2enu function will also need a test itself since there is nothing about it and any possible regressions will not be testable.

As always, all the above are my opinion about it 🙂. I'm not the gatekeeper anymore so others may have a different opinion. There are some extra points I noted too but they are minor in comparsion to the above.

PS: The changes in binMapping are likely wrong. The correct way here is probably to calculate the absolute value or reverse the dvar/beam variables given the shift in orientation. A modification here will require tests too to avoid/catch regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants