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

Fuse Coreg point functions and add consistent Raster-Point logic #480

Merged
merged 57 commits into from
Mar 29, 2024

Conversation

rhugonnet
Copy link
Contributor

@rhugonnet rhugonnet commented Mar 1, 2024

Summary

This PR adds point-raster and point-point support in the base Coreg class, with modifications to properly inherit it through CoregPipeline and BlockwiseCoreg, making the treatment of point-raster coregistration fully consistent for the entire coregistration module. It adds point-raster support for all BiasCorr classes, and maintains current point-raster support for AffineCoreg classes.

In practice

Any gpd.GeoDataFrame can now be passed with a z_name argument defining the column name representing the elevation point cloud data, and it can be passed either as reference_elev, or to_be_aligned_elev, or both. In the future, it will be easy to pass an xdem.ElevationPointCloud() that knows the z_name to avoid multiplying arguments, see #463.

A gpd.GeoDataFrame is always compared to a gu.Raster using gu.Raster.interp_points, fully consistent and tested since GlacioHack/geoutils#484. It mirrors Xarray.interp() behaviour, but should be faster on equal grids thanks to using directly map_coordinates.
A gu.Raster is converted into points using to_points() (for tests or for fallback mechanism in the Coreg class discussed in #402). This still requires some fixes in GeoUtils to avoid passing too many arguments: GlacioHack/geoutils#499.

Everything is done through fit() and apply().

Changes

This PR:

  • Merges the functions apply_pts and apply, and fit_pts and fit,
  • Moves the pre/post-processing of fit or apply into non-public _preproc_... and _postproc_... functions for better organisation and clarity depending on input type (point or raster),
  • Renames reference_dem and dem_to_be_aligned into reference_elev and to_be_aligned_elev as inputs can be DEMs or elevation point clouds,
  • Renames _fit_func() into _fit_rst_rst() (only valid for "Raster-Raster") for all subclasses, _fit_pts_func() into _fit_rst_pts() (for "Raster-Point"), and adds a _fit_pts_pts() (for "Point-Point"); also renames _apply_func() into _apply_rst() (apply to a raster) and _apply_pts_func() into _apply_pts() (to point),
  • Adds the fallback established by @erikmannerfelt discussed in Revamp and simplify the raster-point/P-R/P-P functionality #402 in a single parent Coreg._fit_func() which distributes to all other fit_rst_rst(), fit_rst_pts() and fit_pts_pts() of subclasses. Same for Coreg._apply_func() that distributes to _apply_rst() and _apply_pts() depending on input type.
  • Changes accepted input point type to only gpd.GeoDataFrame with a column name z_name (in the future will easily accept gu.PointCloud that can be created easily from an array and read from a point cloud file e.g. LAZ, but need to wait for GeoUtils PRs for this),
  • Replace CoregPipeline._apply_rst() function by an overridden apply() function to work properly for any point/raster input and with BlockwiseCoreg, this mirrors the overridden fit() function added in Improve subsample across Coreg subclasses and pipelines #436,
  • Makes relevant changes to raster-point functions _fit_rst_pts() that had inconsistent input/output,
  • Renames the apply_matrix() function into _apply_matrix_rst(), and moves the code used to apply matrix transforms to point data into a new _apply_matrix_pts() function, and wraps them into a new apply_matrix() function that works on any raster or point! Also makes invert_matrix() public through coreg,
  • Renames BiasCorr.fit_rst_rst() into BiasCorr._fit_biascorr() to be able to call it through BiasCorr.fit_rst_pts() without being overridden by subclass functions, and now calls directly this parent class function in most subclasses, also renamed in BiasCorr1D, 2D, etc, to maintain the chain of checks,
  • Adds a subsample_final key in Coreg._meta to save the number of final pixels used during Coreg, see Store final number of points used by subsample in Coreg class #479,
  • Creates a NotImplementCoregFit and NotImplementedCoregApply subclasses of NotImplementedError to differentiate these specific type of errors made to track the implementation of Coreg subclass methods to other potential NotImplementedError.
  • Adds tests in test_biascorr to check any combination of raster-raster, raster-point or point-raster input for all functionalities, with checks verifying that all corrections indeed remove 99% of the variance introduced artificially,
  • Makes minor adjusts in other tests of test_coreg/, almost everything was passing the same way with the new structure!
  • Fixes the GradientDescending test values by half a pixel due to previous enforcement of "area" or "point" coordinates,
  • Improving tests of test_affine will have to come in another PR, see below.

Additionally, this PR makes all UserWarning fail tests at the package level, and removes the individual warnings.simplefilter("error"). All other warnings that depend on our routines are either fixed, filtered or caught if expected during the tests.

Resolves #472
Resolves #479
Resolves #402
Resolves #475
Resolves #134
Resolves #404
Resolves #489
Resolves #376
Resolves #377

TO-DO:

  • Add point-raster support in biascorr,
  • Add point-raster tests in test_biascorr,
  • Fix point-raster behaviour for CoregPipeline.

For later

Reworking tests in test_affine and adapting some of the functions: more efficient if done in another PR after working on the AffineCoreg restructuration for being more modular. Opened #485.

@rhugonnet rhugonnet marked this pull request as ready for review March 7, 2024 01:00
@rhugonnet
Copy link
Contributor Author

rhugonnet commented Mar 7, 2024

Tests all passing locally! 🥳
To fix them in CI: probably need to release a 0.1.1 for GeoUtils that integrates the recent interp_points fixes!

@rhugonnet
Copy link
Contributor Author

@erikmannerfelt Long time since we had that revamping discussion, it's finally done!! 😁

@rhugonnet rhugonnet changed the title Fuse Coreg point functions and add Raster-Point logic Fuse Coreg point functions and add consistent Raster-Point logic Mar 7, 2024
@rhugonnet
Copy link
Contributor Author

rhugonnet commented Mar 12, 2024

Tests pass locally but require GlacioHack/geoutils#501

xdem/coreg/base.py Outdated Show resolved Hide resolved
xdem/coreg/base.py Outdated Show resolved Hide resolved
xdem/coreg/affine.py Outdated Show resolved Hide resolved
xdem/coreg/biascorr.py Outdated Show resolved Hide resolved
xdem/coreg/biascorr.py Outdated Show resolved Hide resolved
adehecq
adehecq previously approved these changes Mar 28, 2024
Copy link
Member

@adehecq adehecq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, impressive work !! Thanks ! 🙏
It's neat that the coreg methods now work similarly for raster or point data! It seems all good to me, I didn't see any big problem, just some minor comments.
I haven't checked the tests and will do later, but you don't need to wait for that.

@rhugonnet
Copy link
Contributor Author

Alelujah OpenCV finally installs on CI again... I've opened a Wiki page to remember how to solve this: https://github.com/GlacioHack/xdem/wiki/Installing-OpenCV-in-CI.

@rhugonnet rhugonnet merged commit 663a702 into GlacioHack:main Mar 29, 2024
13 checks passed
@rhugonnet rhugonnet deleted the fuse_coreg_pts branch March 29, 2024 17:52
@adehecq
Copy link
Member

adehecq commented Apr 12, 2024

Just thinking: should we add a point cloud to our sample data to document these new features in the documentation?

@rhugonnet
Copy link
Contributor Author

Yes, maybe once we have #463!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment