-
Notifications
You must be signed in to change notification settings - Fork 42
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
Re-structure coregistration affine methods, point-raster pre-processing, binning and fitting, and add pixel interpretation support #530
Conversation
AffineCoreg
subclasses to be more modularAffineCoreg
subclasses to be more consistent and modular
AffineCoreg
subclasses to be more consistent and modular
@adehecq @erikmannerfelt This PR is ready for your review! I have two main aspects on which I'm hesitating on how to move forward, and would especially like your opinion on:
|
Agreed on all. For computation times: Similar for Raster-Raster, and faster for Point-Raster (due to avoiding rebuilding a Raster to call Good idea, I will try to write a short Wiki page why this is fresh! |
Merging like this! |
FYI: I've added a Wiki page here on the |
Perfect, thanks a lot ! That's super useful ! |
This PR restructures affine coregistration methods and many generic subfunctions to be consistent and modular.
Primarily, it makes 1/ "point-raster" subsampling, 2/ any binning and/or fitting, and 3/ iteration over methods, the same across all
Affine
andBiasCorr
methods with extensive modularity.Additionally, this PR updates horizontal shift reprojection from using a different implementation in every method (e.g. Nuth and Kääb used
scipy.RectBivariateSpline
) to using consistentlyinterp
from GeoUtils which supports any resampling method (for which details can be chosen in globalconfig
parameters), with consistent nodata propagation built on top, and preserves computational efficiency by allowing to return a single interpolator object re-used (GlacioHack/geoutils#560). This also allows to avoid sub-pixel errors of Rasterio reprojection.It also re-structures the code of affine methods, defining core functions outside the classes'
_fit_rst_rst
or_fit_rst_pts
, which allows to have consistent pre-processing steps, and clear input/outputs.Finally, this PR adds support for pixel interpretation for coregistration methods (important for point-raster operations).
Resolves #234
Resolves #574
Resolves #561
Resolves #546
Resolves #573
Resolves #571
Resolves #547
Resolves #327 (except plot but those are mentioned in #139)
Big step forward for #435
Opens #572
Opens #575
Opens #579
Summary of changes
1/ New core processing functions
For consistent binning/fitting:
Restructures the
coreg/affine
module to support thefit_or_bin
logic previously added incoreg/biascorr
: However, as in the case of coregistration, the fit function is fixed (that of the method; for instance dh/slopetan = aspect for Nuth and Kääb), the "bin" option alone is impossible and only "fit" or "bin_and_fit" are supported. Therefore, the mirrored structure simply allows to pass a boolean argumentbin_before_fit
to perform binning before fitting, as well as any fit optimizer, bin statistic, and bin sizes.To do this, this PR modifies the bin and fit of
BiasCorr
into a single_bin_or_and_fit_nd
function incoreg.base
used by allAffine
andBiasCorr
methods.For consistent point-raster/raster-raster subsampling:
This pre-processing step is different from that running directly in
Coreg.fit()
, which simply extracts thetransform
,crs
, and others. This step revolves around subsampling the point-raster (raster-raster, or point-raster) at the same points for a valid subsample (no NaNs) of a certain size. This cannot be done consistently infit()
before_fit_xxx
, because auxiliary variables might need to be derived by the method on the full arrays (such as slope, aspect, etc), which propagate NaNs. In particular, the tricky aspect is the case of point-raster, where we need to first interpolate the raster at the coordinates of the points to get an idea of the available valid subsample (depending here also on NaN propagation).To do this, this PR modifies the subsampling that existed in
BiasCorr
into a single_preprocess_pts_rst_subsample
function incoreg.base
relying on two steps:_get_subsample_mask_pts_rst
and_subsample_on_mask
. In order to allow to return a dh interpolator that can be re-used iteratively for some affine methods, another function_preprocess_pts_rst_subsample_with_dhinterpolator
incoreg.affine
relies on a different second step_subsample_on_mask_with_dhinterpolator
.For consistent iterative methods:
Methods such as ICP, Nuth and Kääb, etc, all work based on iterations. To have consistent treatment of inputs/outputs and iterative arguments (tolerance to reach, maximum number of iterations), those are now grouped through an
_iterate_method
function.Ensuring consistent argument across these core functions and other parameters:
To ensure the argument passed to these three core functions (subsampling, binning/fitting, and iteration) are consistent, new dictionary types are defined for each which defines their key names and value types:
InFitOrBinDict
("In" for input),InRandomDict
andInIterativeDict
. AdditionallyInAffineDict
for affine-related parameters, andInSpecificDict
for any method-specific parameters that does not fit other categories.Same is done for outputs:
OutRandomDict
, etc.See #573 for details on the new organization.
2/ Robust and consistent horizontal shift reprojection
The core of
fit_
functions of horizontal methods such asNuthKaab
andGradientDescending
has been modify to rely on the_preprocess_pts_rst_subsample_with_dhinterpolator
mentioned above.This function relies on
geoutils.interp_points
(directly for point-raster, and through a new function_reproject_horizontal_shift_samecrs
for raster-raster) to perform a consistent 2D grid interpolation with nodata propagation supporting any SciPy method, see details here: GlacioHack/geoutils#560.This PR adds a test checking that the result of this functionality matches GDAL reprojection, which ensures Rasterio sub-pixel reprojection errors are gone.
3/ Re-structuration of affine classes
All code from affine classes is moved out of the
_fit_xxx
functions into functions that rely on the same steps where possible.For instance, the
NuthKaab
method is divided into a_nuthkaab_fit_func
to define the function to optimize, a_nuthkaab_fit
which wraps_bin_or_and_fit_nd
with some initial guess, a_nuthkaab_aux_vars
that derives auxiliary variables required for the fit (slope, aspect), and a_nuthkaab_iteration_step
that is passed to_iterate_method
.All the
_apply_xxx
affine functions are removed to avoid unnecessary redundancies/inconsistencies, as theapply_matrix
is the same across all affine methods, and since #531 supportsresample=False
that modifies only the geotransform for horizontal shifts.4/ Add support for pixel interpretation
Dealing with
area_or_point
was previously done arbitrarily in each subclass. Using the above interpolation/reprojection (throughgeoutils.interp_points
), we can now simply pass the argument to GeoUtils who accounts for it when comparing point-raster, or raises a warning for raster-raster having a different interpretation.It is only required for the
fit
part of coregistration, and thus added to all related functions.5/ Removal of
Tilt
, and other subfunctionsWith the above changes, the affine
Tilt
function corresponds exactly toDeramp(order=1)
(instead of having its own, separate implementation). Additionally, it is not truly affine (cannot be modelled by an affine matrix), and thus has no place in affine methods. We can simply remove, and users can rely onDeramp
instead.All sub-functions that were created to perform some processing steps in coregistration modules (many times duplicated from another method, or from a GeoUtils functionality) for a specific affine method are now removed. This is largely thanks to GlacioHack/geoutils#560 which re-structures GeoUtils raster functionalities to be called without having to build a Raster object (simply passing array, transform, CRS, etc).
Until now, we were deconstructing/reconstructing Raster objects everywhere to call simple functions (
interp_points
,coords
, etc), which made us lose performance. This is now largely solved too!A couple exceptions remains:
warp_dem
used only byBlockwiseCoreg
and_mask_as_array
used only by thedDEM
class, but their use seems more specific and I'm not sure it is covered by anything in GeoUtils right now.Tests
I did not write new tests on purpose in this PR to ensure I could check that all these changes do not affect our current methods significantly. Improving tests (in particular for affine methods) will be addressed in a next PR.
And indeed, all tests are passing with minor modifications! 🥳
Additionally, I increased the speed of the tests following #574 (ICP tests were extremely slow and cumulated to 5min+, or half of xDEM's test length). Now nearly all the tests of
test_coreg/
run on a cropped version of the reference/tba example DEMs.And all of xDEM's test run in 5min instead of 12min+. 😃
To-do-list
NuthKaab
of Bug with raster-point coregistration #546,interp_points
and allow to return interpolator geoutils#560,BiasCorr
subclasses),Affine
functions out of the classes, and adapt them to work with the new generic core functions,is_translation
property to trigger error for unsupportedresample=False
,Coreg.info()
to summarize coregistration information, inputs and outputs._check_bin_or_fit
function for affine classes (opened Add consistent checks for arguments of coregistration classes #575).Will do in separate PRs
test_affine
once core class restructured withBiasCorr
parameters #485,0.1
release #502.