Bug binmapping

[evacougnon]

Originally submitted by @BecCowley
Supersedes #755

This fix is a sign adjustment to the bin mapping algorithm based on the direction the instrument is facing. Current code is only correct for up-facing ADCPs.
Also adds the pitch gimbal correction.

@BecCowley , I resubmitted your PR to only include the relevant commits. Note that I divided one of your commit in 2 (Add pitch adjustment calculation and add upward looking definitions). I also added a clarification in the comments describing the orientation of the RDI 4 beams ADCPs, let me know if you're happy with that:

For an upward looking ADCP, when pitch is positive beam 3 is closer to the surface while beam 4 gets further away. When roll is positive beam 2 is closer to the surface while beam 1 gets further away.

As adcpBinMappingPP.m also deals with Nortek ADCPs, @sspagnol could you test this PR with one of your downward facing Nortek ADCP please? If I'm not wrong you mostly have Nortek ADCPs. I was only able to test it with a Nortek Continental (upward looking).

For reference on this topic, here is some information worth keeping: #753

[ggalibert]

Code looks ok to me. I would squash together the following commits:

• update comment describing RDI 4 beams ADCP
• add upward looking definitions
• Bug fix to allow for down-facing instrument bin mapping for RDI 4 beams

They are all part of the same fix.

I agree the last commit should be separate as this is to address a different issue as far I understand.

Happy to merge if you had some positive feedback from Simon and if both Simon and Bec agree with the extra step to adjust pitch.

[ggalibert]

As suggested by Hugo, instead of declaring the variable above and then later declare signs for both pitch and roll while only pitch changes sign depending on whether the ADCP is facing up or down, you could just:

• declare pitchSign = 1 here,
• then pitchSign = -1 in the if statement below,
• finally just use pitchSign in the equations line 123 and 124 below

[ggalibert]

I suspect this line and below are only going to work for RDI ADCPs and will break for Norteks. If it is tru that the current PITCH variable is not the best one and that instead we should consider the adjusted pitch from the gimbal then it should be fixed somewhere else when assigning the content of the PITCH variable. What does Bec think about this? Is there a similar situation with Nortek?

[sspagnol]

Comment from the sidelines of someone who is not a Nortek export :)

From nortek support group comment here and oceannetworks processing comments here I had assumed that RDI gimballed pitch correction does not apply to Norteks.

And more a style comment - I sometime get lost in long functions with lots of maths :) A long time ago when playing around with this code I had started structuring as

if isRDI% RDI 4 beams
% handle 4 beams

% TODO: 5th beam

elseif isNortek
   if number_of_beams == 3
   % current 3 beam code
   else
   % new 4-beam code
    end
end

So maybe after this patch this sort of thing can be done. And then maybe break out the if instrument_type parts to say Preprocessing/+TeledyneADCP/calculate_non_binmapped_heights.m and Preprocessing/+Nortek/calculate_non_binmapped_heights.m. It may end up the same transformation for 4-beam Nortek as an RDI, haven't looked at it for so long couldn't even tell you if the beam numbering is the same let alone its relation to pitch/roll. Just would prefer more splitting out.

This is purely selfish as I know one day someone is going to through more complex Rowe Technologies adcp data at me.

[BecCowley]

I suspect this is just a RDI correction. I don't know about Norteks, but I suggest changing it to only apply to RDI data and sort out the Norteks separately.

[hugo-sardi]

@ggalibert is correct here - The line mentioned only works for RDI. The "sensor_settings" fieldname is only available for RDIs, and this key-value lookup was implemented when doing the beam2earth functionality.

[ggalibert]

Commit for adjusted pitch shouldn't be part of this PR. It is not ready yet and this issue should be addressed somewhere else than in the bin mapping code. We may need a PP routine to adjust pitch values depending on orientation as suggested by Hugo but we need to confirm this either from documentation, manufacturer, comparison with manufacturer bin mapping results. Is this relevant for both RDI and Nortek?

[BecCowley]

@evacougnon, having trouble adding a nice clean commit to this branch with the updates to binmapping for down-facing instruments as discussed with @ggalibert

Attached the binMappingPP.m with hacks. Ultimately, they make very little difference if included or not. And, they could be done better.
adcpBinMappingPP.txt

[hugo-sardi]

as mentioned in the email, the pitch correction/gimballed pitch opened a can of worms here. IMO, The correct flow here is to split the code into a new PP that applied the pitch correction and propagate/clobber the pitch variable down the line. This is easily testable and composable (e.g. binmapping with/without the pitch correction).

[petejan]

I disagree, the pitch report in the netCDF file should be as measured, not as used in the calculation. any use of the pitch variable should be aware what the pitch is (gimballed or fixed).

[BecCowley]

@petejan, the calculation is dependant on the way the tilt sensors are fixed to the ADCP. I have attached a very old document from RDI that describes the three different options. I am not an expert, but assume our instruments are all case 1 on page C-6.
Appendix.C.-.Coordinate.XFORM.Trig.Functions.pdf

I did test the effect of the correction on the outcomes, and there was very little difference if it was applied or not.

[petejan]

@BecCowley yes I would agree its case 1, both the pitch and roll are fixed to the ADCP. The correction is small

roll	pitch	KA	gimbal pitch	cos(pitch)
0	0	1	0	1
10	0	1	0	1
0	10	1	10	0.984808
10	10	0.999545	-8.3945325	0.989286
40	40	0.910651	-29.2949941	0.872112

At 10 degrees pitch/roll its only 2%, or 1.4 degrees, out at 40 degrees its 13 % or 10.3 degrees, the angle on the ADCP is probably only good to 6 degrees.

[BecCowley]

@petejan thanks for those calculations. Maybe it makes more of a difference than I realised. I thought I'd checked it with the real data I have and saw very little difference in the bin-mapping outcomes at 26 degrees tilt, but maybe I stuffed up.

[hugo-sardi]

I disagree, the pitch report in the netCDF file should be as measured, not as used in the calculation.

The best way is to propagate in a different variable or take the raw instrument for what it is - RAW_PITCH - and modify (with renaming) it along the way according to the user's needs if required. Ideally, we want to keep the variable PITCH at the end since compatibility matters.

However, the number of changes (testing) may be excessive (hard), particularly if the change is only forced by one kind of instrument. For the record, the clobbering solution is not inconsistent with the behaviour at the PP/FV01 level (or even at the parser level, if you consider unit conversion is done). In a sense, one could accept that an added comment to the PITCH variable is enough to signify a small correction/clobbering of it. This is done with say pressure and with binmapped velocities at binMappingPP. Is anyone complaining that Velocities are clobbered after binMapping at FV01 level instead of being renamed to UVEL_BMAP? One needs to take into account that, at the FV00 (FV01) level, the original data is kept (modified already).

any use of the pitch variable should be aware what the pitch is (gimballed or fixed).

I agree. For the sake of the argument, I didn't mean that we shall clobber the PITCH variable blindly, but clobbering it with a comment added is not unheard of in the codebase and it is a quick solution.

[petejan]

@petejan thanks for those calculations. Maybe it makes more of a difference than I realised. I thought I'd checked it with the real data I have and saw very little difference in the bin-mapping outcomes at 26 degrees tilt, but maybe I stuffed up.

@BecCowley you may only see a difference when the roll and pitch are large (and the same sign).

[petejan]

@hugo-sardi It was not really a question of clobbering or not (clobbering has its uses in certain cases) but should we keep intermediate calculations. For RDI mooring mounted ADCP the pitch axis is always fixed to the ADCP and not gimballed.

A comment in the pitch variable would be good to indicate this, and comment in the Nortek parser about which way the Nortek is also. The only case I can imagine is the Nortek with the AHRS may not be gimballed, and where the ADCP is installed on a ship and the pitch/roll is comming from the ships systems, this is a very different use case.

[evacougnon]

@BecCowley I included the last changes you sent above and I removed the section related to the "adjust pitch" code that will be part of a different PR, currently as a draft including the section of code you suggested. I refactored the BinMapping, so the adjusted height above sensor calculation is now a block function, as it was suggested somewhere.

@lbesnard ready for you to have a look