-
Notifications
You must be signed in to change notification settings - Fork 42
Add CPD, LZD and make ICP Python-based #701
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
base: main
Are you sure you want to change the base?
Conversation
…ption, fix other options and add tests
@adehecq @erikmannerfelt @adebardo @belletva @vschaffn This PR is ready for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just by going through the doc, I'm thinking that when we describe "VerticalShift", maybe we should point readers to the "Vertical referencing" section to avoid having people using this method to correct for differences in vertical referencing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe one needs to update the "recommended pipelines" section? As it does not include the new methods.
|
||
{class}`xdem.coreg.ICP` | ||
{class}`xdem.coreg.LZD` | ||
|
||
- **Performs:** Rigid transform transformation (3D translation + 3D rotation). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should "Rigid transform transformation" be "Rigid transformation"? It appears several times.
@@ -476,6 +550,8 @@ For both **inputs** and **outputs**, four consistent categories of metadata are | |||
|
|||
**4. Affine metadata (common to all affine methods)**: | |||
|
|||
- An input `only_translation` to define if a coregistration should solve only for translations, | |||
- An input `standardize` to define if the input data should be standardized to the unit sphere before coregistration, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not super clear. Is it better described somewhere, that you could point to?
with warnings.catch_warnings(): | ||
# Deprecation warning from pytransform3d. Let's hope that is fixed in the near future. | ||
warnings.filterwarnings("ignore", message="`np.float` is a deprecated alias for the builtin `float`") | ||
|
||
# return np.linalg.inv(matrix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be removed?
checked_matrix = pytransform3d.transformations.check_transform(matrix) | ||
# Invert the transform if wanted. | ||
return pytransform3d.transformations.invert_transform(checked_matrix) | ||
|
||
|
||
def _apply_matrix_pts_mat( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what this function does. Could you add a quick explanation in the docstring?
assert norm is not None | ||
diffs = (trans_tba - ref) * norm | ||
else: | ||
raise ValueError("ICP method must be 'point-to-point' or 'point-to-plane'.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this should be caught at the beginning of the function
# Define residuals depending on type of ICP method | ||
# Point-to-point is simply the difference, from Besl and McKay (1992), https://doi.org/10.1117/12.57955 | ||
if method == "point-to-point": | ||
diffs = (trans_tba - ref) ** 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused about this part. These are point clouds right? I.e. an array of 3xN, but why would N be the same in both the tba and ref?
I always assumed that ICP requires finding the nearest neighbor in the other point cloud, not just taking the difference point to point.
But I guess it must work since your results compare to other implementations. I just don't understand why... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, from what I understand after looking at the functions below, at this stage, the two arrays contain the nearest neighbors for the two PC already?
def centroid(self) -> tuple[float, float, float] | None: | ||
"""Get the centroid of the coregistration, if defined.""" | ||
meta_centroid = self._meta["outputs"]["affine"].get("centroid") | ||
# For this type of method, the procedure can only be fit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be deleted?
Based on Besl and McKay (1992), https://doi.org/10.1117/12.57955 for "point-to-point" and on | ||
Chen and Medioni (1992), https://doi.org/10.1016/0262-8856(92)90066-C for "point-to-plane". | ||
|
||
The function assumes we have a two DEMs, or DEM and an elevation point cloud, in the same CRS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo "a two" -> two
@adehecq made the remark that it is hard to get into the structure of coregistration function such as |
This PR adds Coherent Point Drift (CPD) and Least Z-difference (LZD) coregistrations and modifies Iterative Closest Point (ICP) coregistration to be only NumPy/SciPy-based. This last addition allows to remove the (heavy) OpenCV optional dependency previously used for ICP, and to call consistent fit optimizer and iterative parameters in ICP, comparable to that of other methods.
All three methods are added in the same PR as they use several common subroutines related to rigid point registration.
This PR takes over #581, which is closed. Thanks to the homogenization in #530, we can now use the same core functions as in other coregs at all steps.
Finally, to ensure our ICP/CPD implementations perform well, they are compared to that of other packages (Open3D, OpenCV, pycpd) in a separate module (that could be moved into its own small repository).
The changes in documentation can be visualized here: https://xdem-rhugonnet.readthedocs.io/en/add_cpd/coregistration.html
Resolves #100
Resolves #483
Resolves #587
Resolves #705
Resolves #706
Background on the methods
The three methods are rigid transformation methods: translations + rotations (CPD can support more complex transforms, but those are not relevant for DEM applications).
ICP and CPD are the two most successful methods of point-point registration, widely used in computer vision and other fields.
LZD is the most successful method of grid-point registration, a much smaller field, mostly used in geoscience. After some investigation, it appears that the Nuth and Kaab (2011) method is actually a special case of LZD for translations only (exact same equation, NK expressed in polar coordinates while LZD is expressed in cartesian coordinates).
Why our own implementations?
I think this is an important question to address first for ICP/CPD where other implementations exist.
The short answer is that, because we need very fine sub-pixel alignment for DEMs (and that ICP/CPD are very sensitive to this), having a modular implementation is key to reduce errors (while it is less important in e.g. computer vision applications).
Existing implementations of ICP in Python (e.g., https://github.com/pglira/simpleICP) don't allow for the modularity we need in xDEM, such as passing any fit optimizer, changing subsampling, or running the various existing optimizations of ICP (that exist in the OpenCV implementation we previously relied on). After inspecting the code in simpleICP, it would be difficult to add the modularity we need there.
Consequently, our
coreg/affine
methods are becoming a bit big for xDEM. They are applicable to any grid-point or point-point rigid registration problem, not just DEMs. And, to my knowledge, there is no such package doing this in Python.Therefore, we might think about taking these methods out of xDEM in the long term, for better maintenance and to provide these modular implementations that can be benchmarked by simulation (explained later) for the larger community.
Changes in this PR
ICP
This PR adds a NumPy-SciPy ICP (nearest point done in SciPy, and optimizer defaults to SciPy but can be anything user-defined, like for other methods), and also adds many improvements of ICP to ensure an accuracy equal to that of OpenCV or Open3D's implementation.
Those are:
scipy.optimize.least_squares
),CPD
This PR adds a NumPy-based implementation of CPD based on the algorithm described in Myronenko and Song (2010), https://doi.org/10.1109/TPAMI.2010.46, and inspired by pycpd's rigid implementation: https://github.com/siavashk/pycpd/blob/master/pycpd/rigid_registration.py.
CPD can have trouble with numerics, so we add a
standardize
option for point-point methods to properly use it, see below.LZD
This PR adds the Least Z-difference coregistration method of Rosenholm and Torlegård (1988): https://www.asprs.org/wp-content/uploads/pers/1988journal/oct/1988_oct_1385-1389.pdf.
Other changes
This PR implements several other changes:
matrix_from_translations_rotations
andtranslations_rotations_from_matrix
conversion functions, used everywhere in other functions and added to the public API,only_translation
option to ICP/CPD/LZD, which allows to solve the coregistration optimization considering only the translation,_get_subsample_mask_pts_rst
function to never return non-finite values,standardize
option to ICP/CPD which allows to standardize input data before running the algorithm for improved numerics (CPD fails without it).filters.py
(that were not listed in the API), and replaces a couple OpenCV functions used in other modules too (usually to generate synthetic data) using only SciPy functions instead,Tests
This PR adds the CPD/LZD methods to existing affine tests, that ensure that a coregistration methods properly corrects misalignment by comparing the shifts/rotations applied synthetically to a DEM with the coregistration result, for a number of case tests (point-raster input, various amplitude of shifts/rotations, etc).
Benchmarking our ICP/CPD algos
In order to benchmark that our ICP performs as well as other algorithms out there, both in terms of accuracy and speed (I only examplify accuracy below), I've written a Python module that runs ICP from OpenCV, Open3D, and CPD from PyCPD. This is currently on a separate repository (as we don't want these dependencies in xDEM).
As in our tests, we artificially shift/rotate a DEM then correct it back, and compute the percentage of difference between the original shift/rotation and the one found by the coregistration (the dx/dy/dz/dalpha values; green = near 0% difference, red = large difference).
We run 3 cases:
This way, we verify that the accuracy of our ICP is the same as the OpenCV/Open3D ICP, and the one of our CPD the same as the PyCPD one. ICP has trouble resolving small shifts, while CPD has trouble resolving all shifts and Z-axis rotation.
Values are the %age of difference from the real shifts (first three columns) or rotations (last three columns)

We could add that new module into a
xdem-benchmark
or similar, that can punctually install/run other packages and xDEM, to compare the performance of all these algorithms. And we could consider adding other algorithms to this repo, for instance the ICP of py4dgeo (#504), or of libpointmatcher (potentially via ASP too): #11.TO-DO to finalize
matrix_to_translation_rotations
andtranslation_rotation_to_matrix
in tests,