Homogenize sign of translation coregistration and add affine tests #585
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes and homogenizes the sign of translation parameters in coregistrations, and adds consistent tests mirrored from
test_biascorr
to ensure that affine coregistrations behave as intended for all types of input (raster-raster, raster-point, and point-raster).In short: the new tests introduce synthetic shifts, then check that estimated coregistration shifts are exactly the same as those introduced within 1% (10% for ICP), and that applying the coregistration then corrects more than 99% of the variance in the initial elevation difference (95% for ICP). This ensures that the direction of coregistration is the right one, and that there is no other errors introduced in the process.
More background
Before #530, the
_apply_rst
of different method such asNuthKaab
andICP
relied on different code to apply the translation. In some occasions, it called the sameapply_matrix
function, but even then it relied on a differentto_matrix_func
to derive the matrix despite depending on the same parameters (shift_x
,shift_y
,shift_z
).This meant that the signs of shifts stored in
shift_x
,shift_y
, andshift_y
could be inconsistent between methods. Since #530 relies on the sameapply_matrix
everywhere, this resulted in a erroneous coregistration direction for certain input types (point or raster) and certain method, which weren't capture by the older tests.Additional fixes
Now that ICP is tested for all combinations of raster-raster, point-raster and raster-point with a subsample, we can indeed conclude that #422 and #423 indeed came from the
prefilter
issue of our oldgeoutils.interp_points
that was removing data by default, which caused issues for point runs, see GlacioHack/geoutils#484 for details.Resolves #422
Resolves #423
Resolves #485
Resolves #586