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

Add pitch adjustment calculation #779

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

evacougnon
Copy link
Contributor

@evacougnon evacougnon commented Mar 17, 2022

This is still a work in progress. This work was first submitted as part of the bug_binmapping PR.
Relevant discussion can be found in the discussion following this comment

It seems that the pitch adjustment calculation is only relevant for RDI instruments and therefore the code presented here will have to be moved to the relevant section (e.g. part of the parser if this is something that needs to be fixed from the start, or part of a new preprocessing routine).

Other useful information when we get to work on this PR from Hugo:

AFAIK, the original binMapping (before the PR) use the RAW Pitch from the instrument, not the adjusted pitch ("gimbal" in the newest code). Hence, all previous code and data processed used the raw pitch instead of the adjusted pitch (gimbal).
The new binMapping PR uses the adjusted PITCH( call it PITCH_ADJ or PITCH_GIMBAL), which is great but also adjust its signal because of orientation. However, the adjusted value (magnitude/signal) used is not carried forward to the final files, either in the form of an overwritten PITCH variable or a new variable (e.g. PITCH_ADJ).

The implications/questions here are the following:

  • Why do previous code (and currently processed data) don't use the adjusted pitch? Unless there is an indication in the manual that sampled adcp in ENU coordinates change the raw PITCH in the binary file to the gimbal pitch, then the previous code is inconsistent (or the new PR (and the Beam2Enu conversion) can't use the adjusted pitch)*.
  • I call it inconsistency because it is likely that the actual gimbal pitch correction is minor for fixed bottom adcps, but not so for bottom looking/significant variations in pitch. Hence, the current historical data, code, and practice are inconsistent (or wrong) compared to the new proposal.
  • Moreover, the current changes imply that previously processed data will likely differ if reprocessed with the "new" PR. Are the results similar? Regressions tests? Again, at best, it is a minor inconsistency; at worse, it's inaccurate data. I assume the corrections will be negligible if the pitch/roll variations are as well, but I can't say that for every file out there.
  • Finally, we need to propagate the adjusted/corrected PITCH variable forward. Also, BinMapping is not supposed to correct the PITCH variables but just binMapping. Hence, IMO, the best way is to make a new PP (adcpOrientationPP) that will adjust the variables according to valid corrections/orientations/units and use the corrected values directly in the binMapping routine. This will require more touch-ups, though.
    *AFAIK, the PITCH output in the binary file is the "raw pitch", and such, the code should use the gimbal conversion and correction.

@evacougnon evacougnon mentioned this pull request Apr 8, 2022
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