Add Initial shift Capability to DEM Coregistration #650

vschaffn · 2024-11-15T15:04:22Z

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:

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.
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.
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.
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.

xdem/coreg/workflows.py

rhugonnet · 2024-11-15T21:02:44Z

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 · 2024-11-15T23:15:49Z

@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.

xdem/coreg/workflows.py

vschaffn · 2024-11-22T12:52:00Z

@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 · 2024-11-22T13:21:49Z

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

rhugonnet · 2024-11-22T20:41:48Z

xdem/coreg/workflows.py

@@ -173,7 +175,7 @@ def dem_coregistration(
    :param src_dem_path: Path to the input DEM to be coregistered
    :param ref_dem_path: Path to the reference DEM
    :param out_dem_path: Path where to save the coregistered DEM. If set to None (default), will not save to file.
-    :param coreg_method: Coregistration method or pipeline.
+    :param coreg_method: The xdem coregistration method, or pipeline.


I think we can state simply "Coregistration method" here, it's implicit we're working with xDEM 😉

rhugonnet · 2024-11-22T20:44:49Z

xdem/coreg/workflows.py

+    # Add the initial shift to the calculated shift
+    if estimated_initial_shift:
+
+        def update_shift(


Clever the recursive function to take care of the pipelines!

rhugonnet · 2024-11-22T20:54:22Z

tests/test_coreg/test_workflows.py

+            tba_dem_shift, ref_dem, estimated_initial_shift=test_shift_list
+        )
+        tba_dem_shift_test = tba_dem.translate(test_shift_list[0] * tba_dem.res[0], test_shift_list[1] * tba_dem.res[1])
+        assert tba_dem_shift.raster_equal(tba_dem_shift_test)


Reading this test (and looking again at the code in workflows), I realize that the input tba_dem is shifted in-place, which thus modifies the input from the user.
Until now our practice has been to "never authorize the modification of any user input", unless the function has a clear inplace argument (when returning same type as self) or is called set_xxx (when re-setting an attribute).

If that makes sense to you as well, we should return a copy in the workflows function instead of modifying the input, I'll tag where 🙂

rhugonnet · 2024-11-22T20:55:43Z

xdem/coreg/workflows.py

+        shift_y = estimated_initial_shift[1] * src_dem.res[1]
+
+        # Apply the shift to the source dem
+        src_dem.translate(shift_x, shift_y, inplace=True)


As stated above, here we'd need to remove the inplace op to not modify user input.

rhugonnet · 2024-11-22T21:00:10Z

tests/test_coreg/test_workflows.py

+        tba_dem_shift = tba_dem.copy()
+        dem_coreg2, coreg_method2, coreg_stats2, inlier_mask2 = dem_coregistration(
+            tba_dem_shift, ref_dem, coreg_method=coreg_simple, estimated_initial_shift=test_shift_tuple
+        )


I think we might have to pass a random_state=42 everywhere (all the dem_coregistration calls) to avoid possible random failures in CI in the future... 🤔

This is because the output shifts depend on both the initial shift and the one optimized by DhMinimize, which doesn't use all the data but uses a random subsample instead, and can lead to slightly different output. Same for all other methods.
Here it is falling on the exact same value probably because of the internal tolerance of the algorithm, but that might change with a different subsample, or under a different OS.

rhugonnet · 2024-11-22T21:04:34Z

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 · 2024-11-22T21:19:13Z

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.

adebardo reviewed Nov 15, 2024

View reviewed changes

xdem/coreg/workflows.py Show resolved Hide resolved

xdem/coreg/workflows.py Show resolved Hide resolved

vschaffn force-pushed the 603-add_initial_shift branch 2 times, most recently from 72db165 to e7f3724 Compare November 19, 2024 09:10

feat: add a configurable initial shift in dem_coregistration

ec9be3b

adebardo reviewed Nov 22, 2024

View reviewed changes

xdem/coreg/workflows.py Outdated Show resolved Hide resolved

vschaffn force-pushed the 603-add_initial_shift branch from e7f3724 to 6b07e67 Compare November 22, 2024 11:14

vschaffn added 3 commits November 22, 2024 13:42

fix: typing

ff6e437

test: add a shift before coregistration test

d5e4602

docs: add dem_coregistration function in doc

d31d074

vschaffn force-pushed the 603-add_initial_shift branch from 6b07e67 to d31d074 Compare November 22, 2024 12:43

rhugonnet reviewed Nov 22, 2024

View reviewed changes

vschaffn changed the title ~~Add Initial shift Capability to DEM Coregistration in xdem~~ Add Initial shift Capability to DEM Coregistration Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Initial shift Capability to DEM Coregistration #650

Add Initial shift Capability to DEM Coregistration #650

vschaffn commented Nov 15, 2024 •

edited

Loading

rhugonnet commented Nov 15, 2024

rhugonnet commented Nov 15, 2024

adebardo left a comment

vschaffn commented Nov 22, 2024

adebardo commented Nov 22, 2024

rhugonnet Nov 22, 2024

rhugonnet Nov 22, 2024

rhugonnet Nov 22, 2024

rhugonnet Nov 22, 2024

rhugonnet Nov 22, 2024

rhugonnet commented Nov 22, 2024

rhugonnet commented Nov 22, 2024 •

edited

Loading

Add Initial shift Capability to DEM Coregistration #650

Are you sure you want to change the base?

Add Initial shift Capability to DEM Coregistration #650

Conversation

vschaffn commented Nov 15, 2024 • edited Loading

Description:

Key Modifications:

How It Works:

Example:

Documentation

rhugonnet commented Nov 15, 2024

rhugonnet commented Nov 15, 2024

adebardo left a comment

Choose a reason for hiding this comment

vschaffn commented Nov 22, 2024

adebardo commented Nov 22, 2024

rhugonnet Nov 22, 2024

Choose a reason for hiding this comment

rhugonnet Nov 22, 2024

Choose a reason for hiding this comment

rhugonnet Nov 22, 2024

Choose a reason for hiding this comment

rhugonnet Nov 22, 2024

Choose a reason for hiding this comment

rhugonnet Nov 22, 2024

Choose a reason for hiding this comment

rhugonnet commented Nov 22, 2024

rhugonnet commented Nov 22, 2024 • edited Loading

vschaffn commented Nov 15, 2024 •

edited

Loading

rhugonnet commented Nov 22, 2024 •

edited

Loading