Bug binmapping

[evacougnon]

Originally submitted by @BecCowley Supersedes https://github.com/aodn/imos-toolbox/pull/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: https://github.com/aodn/imos-toolbox/issues/753

[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