Add Initial shift Capability to DEM Coregistration

[vschaffn]

Resolves #603.

Description:

This PR introduces the ability to specify an initial shift (in X and Y) for DEM co-registration. The key changes include the integration of an estimated_initial_shift parameter in the dem_coregistration() function, allowing users to apply a custom translation (affine-based) to the source DEM before coregistration.

Key Modifications:

1. dem_coregistration() function:

   • A new estimated_initial_shift parameter is added, allowing users to input a list representing the initial shifts along the X and Y axes.
   • If a shift is provided, the source DEM is translated using geoutils.translate() based on the calculated pixel resolution.
   • The shift_x and shift_y values in the coregistration method’s metadata (meta['outputs']['affine']) are updated to include the initial shifts.

2. Raise TypeError if the coregistration method or one of the coregistration method in the coregistration pipeline is not affine when an initial shift is provided.

3. Recursive Shift Update Logic:

   • A recursive update_shift() function is implemented to handle both Coreg and CoregPipeline objects, ensuring that the shift is propagated across all coregistration steps in a pipeline.
   • Metadata handling ensures shifts are correctly updated in all pipeline steps and for standalone Coreg objects.

4. Unit Tests:

   • Added a test to verify the correct behavior of the estimated_initial_shift parameter.
   • Added a test to ensure TypeError is raised an initial shift is provided and the coregistration method is not affine.

How It Works:

• The estimated_initial_shift input, given as a list [x_shift, y_shift], is multiplied by the DEM’s pixel resolution to calculate the translation in map units.
• This shift is then applied before coregistration, and the resulting shift_x and shift_y in the meta are updated accordingly.
• For CoregPipeline, this process is repeated for each coregistration step to ensure consistency.

Example:

dem_coregistration(
    src_dem_path="path/to/src_dem.tif",
    ref_dem_path="path/to/ref_dem.tif",
    estimated_initial_shift=[10, 5]
)

In this case, the source DEM is initially translated by 10 pixels in the X direction and 5 pixels in the Y direction before the coregistration process begins.

Documentation

• Updated the function docstring to reflect the new estimated_initial_shift parameter and its usage.
• Update API documentation.

[rhugonnet]

Quick note, the tests you are adding are in a test_ function currently skipped because of an old segfault:

xdem/tests/test_coreg/test_workflows.py

Line 186 in 22dbe7c

@pytest.mark.skip(reason="The test segfaults locally and in CI (2023-08-21)") # type: ignore

I see some code in this test that is now deprecated (for example using == to check raster equality was an old choice, now changed to raster_equal() to mirror NumPy and leave == for element-wise equality, as for arrays). I will update that quickly in a separate PR and see if I can make the original test pass 😉

[rhugonnet]

@vschaffn I managed to update the code for the test and unskip it. I merged it to main so that you can update with upstream and run it in this PR 😉: #651.
(The segfault was likely not from us but a dependency, so I didn't have to do anything on this).

[adebardo]

I can't remember if we talked about adding a section in the documentation.

[vschaffn]

@adebardo I added xdem.coreg.workflows.dem_coregistration in API documentation, and a raise when an estimated shift is provided but the coregistration method is not affine

[adebardo]

@duboise-cnes @rhugonnet it's good for you ?

[rhugonnet]

This is great, thanks! Just a couple comments above that can be solved quickly 😄

(Can also remove "xdem" in the PR title, to echo the comment on the argument description!)

[rhugonnet]

Additionally, I think this PR raises a question for future discussion (no need to answer this to merge this PR, just for the record):
Do we want to conserve both DEM.coregister_3d() and coreg.dem_coregistration() in the long term?

Recently we had been moving all functions to be only object method like DEM.slope(), DEM.coregister_3d() to mirror Xarray/GeoPandas behaviour, and naturally integrate them once the accessors for both are here (#656), removing others from the API (like xdem.terrain.slope()).

It's simply about moving the functionality elsewhere, so minimal effort once we have decided on a format.
Tagging @adehecq so that he sees this once he's back (as he didn't know about the recent coregister_3d option I think). I think I'd be in favour of moving dem_coregistration into DEM.coregister_3d.

[vschaffn]

@rhugonnet many thanks for your feedback.

• As copying the DEM at translation step would have triggered refactoring issues, I simply copied the DEM at the begging of the function (if a DEM is provided, if it is a path it is not applicable as the raster created in the function will not go through), so that the DEM is not modified by the function (I added a test to ensure that).
• For the tests I have noticed the random aspect in coregistration methods, but as I instantiate them before use, I have no trouble with that. But if the test does not pass with you I agree.
• It seems logical to keep only one coregistration function, as xdem is moving towards object-oriented design, it would make sense to keep DEM.coregister_3d(), but it means that it won't be possible anymore to compute coregistrations from paths only

[rhugonnet]

@vschaffn Perfect!

For the tests I have noticed the random aspect in coregistration methods, but as I instantiate them before use, I have no trouble with that.

Actually the random_state does not depend on instantiation, it's defined during fit(). So you will have a different randomized output with the same Coreg instance for different fit() calls! (or with different Coreg instances too; basically all the time except if you pass random_state to fit()).

There was a different small issue with dem_coregistration due the way the default argument was defined (that this discussion reminded me of), I used the opportunity to fix it: #662

[vschaffn]

@rhugonnet thanks for your modifications. I added a random_state=42 each time I use dem_coregistration in test_workflows 😃