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

Homogenize sign of translation coregistration and add affine tests #585

Merged
merged 6 commits into from
Sep 11, 2024

Conversation

rhugonnet
Copy link
Member

@rhugonnet rhugonnet commented Sep 10, 2024

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 as NuthKaab and ICP relied on different code to apply the translation. In some occasions, it called the same apply_matrix function, but even then it relied on a different to_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, and shift_y could be inconsistent between methods. Since #530 relies on the same apply_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 old geoutils.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

@rhugonnet rhugonnet changed the title Homogenize apply sign of translation coregistration and add affine tests Homogenize sign of translation coregistration and add affine tests Sep 11, 2024
@rhugonnet
Copy link
Member Author

@adehecq Merging this to use it to finalize #502, as it's mostly tests and your comments can easily be accounted for in a separate PR!

@rhugonnet rhugonnet marked this pull request as ready for review September 11, 2024 22:22
@rhugonnet rhugonnet merged commit 34e5049 into GlacioHack:main Sep 11, 2024
22 checks passed
@rhugonnet rhugonnet deleted the test_affine branch September 11, 2024 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment