hugo-sardi
commented
3 years ago The ADCP pre-processing functions adcpWorkhorseVelocityBeam2EnuPP and adcpBinMappingPP code split seems wrong.
I suppose you are talking about splitting the original PR code submitted right? It was like that to reduce code bloat, enforce good design/separation of concerns, and testable code.
Long story: The original PR that transform the code to ENU got the same lines from adcpBinMappingPP baked inside. The code was not quite testable, since any internal function to any function is not reachable outside of the top function scope. Finally, the original bin mapping baked inside was not doing tilt correction at all and wasting computer cycles.
I see the flexibility of applying (or not) a tilt correction, as a feature, not a bug. This way, users have total control of the PP step for the conversion, and can, for example, verify some bits of calculation with/without the tilt correction for example and "easily" graph the results in the UI ("easily" because we can't still re-run PPs and compare two PP runs in the same session but that is another issue...). Testing is also much easier.
At the moment if adcpBinMappingPP is run without the adcpWorkhorseVelocityBeam2EnuPP then data is bin mapped but still in beam coordinates, even though the new dimension is HEIGHT_ABOVE_SENSOR
Yes, this is explained on the wiki. There are also warnings about it. Please also note that this particular use-case (RDI beam to ENU with tilt correction) is not actually complete, since the original code I received was not right (see my recent comment here and comments on the test/Preprocessing folder )
adcpWorkhorseVelocityBeam2EnuPP is the code should actually converts from Beam Coordinates (DIST_ALONG_BEAMS) to Earth (HEIGHT_ABOVE_SENSOR) so should add the new dimension etc.
adcpBinMapping adds a new dimension because it remaps the vertical position of the bins from zbin(t,z(t)) to zbin(t,z).
The ADCP pre-processing functions adcpWorkhorseVelocityBeam2EnuPP and adcpBinMappingPP code split seems wrong.
adcpBinMappingPP bin maps the data, but creates the new HEIGHT_ABOVE_SENSOR dimension and adcpWorkhorseVelocityBeam2EnuPP applies the pitch/roll/heading to convert to earth coords.
A much clearer split might be
adcpBinMappingPP to just map the bins to the same distance from the sensor (along its axis).
adcpWorkhorseVelocityBeam2EnuPP is the code should actually converts from Beam Coordinates (DIST_ALONG_BEAMS) to Earth (HEIGHT_ABOVE_SENSOR) so should add the new dimension etc.
At the moment if adcpBinMappingPP is run without the adcpWorkhorseVelocityBeam2EnuPP then data is bin mapped but still in beam coordinates, even though the new dimension is HEIGHT_ABOVE_SENSOR