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

Re-organization of coreg.py #327

Closed
rhugonnet opened this issue Oct 25, 2022 · 3 comments · Fixed by #530
Closed

Re-organization of coreg.py #327

rhugonnet opened this issue Oct 25, 2022 · 3 comments · Fixed by #530
Labels
priority Needs to be fixed rapidly

Comments

@rhugonnet
Copy link
Member

rhugonnet commented Oct 25, 2022

Several ideas here in relation to the one-liner coregistration: #267 (review)

More generically:

  • The core functionalities of Coreg methods should live outside of Coreg classes for readability and flexibility (e.g., functions to estimate the horizontal shift, to deramp, etc). In other words all Coreg methods should only be wrappers of already existing functions;
  • Optimization functions should call the methods of fit.py instead of directly calling scipy.optimize, so that their tolerance and outputs are consistently tested, and that any type of optimization method can be used;
  • While all class method should retain flexibility to work with np.ndarray, they should still be optimized to function with Raster objects (e.g., modifying the geotransform instead of resampling in case of a horizontal shift). To have a similar default behaviour for np.ndarray objects, we could potentially introduce a return of a transform object as output of apply().

Maybe @adehecq has more specific things to add!

@adehecq
Copy link
Member

adehecq commented Oct 25, 2022

Yes, I agree with those points. Below I outlined the main steps to be done, hopefully in logical order:

  1. Step 2 requires that the data is convertible to a DEM instance -> Change coreg apply and fit functions so that they accept either an xdem.DEM instance, or a numpy array, transform and CRS.
  2. Replace all Coreg core functionalities with either standalone function or xdem/geoutils functions
    • Replace calculate_slope_and_aspect with xdem.terrain attributes
    • Replace subsampling with geoutils’ subsampling
    • Remove duplicates of deramping
  3. Add function to calculate standard stats before/after coregistration (mean, median, NMAD, std, others?). Call this function in apply.
  4. Add plotting step in all apply methods, and in some fit methods, like N&K.
  5. Update Coreg so that coregistration params are more easily output than currently with _meta
  6. Add function to create inlier mask (load vectors, remove slopes, outliers…)
  7. Save input/output DEM during fit and apply functions to avoid redoing calc?
  8. Remove unused/redundant functions (filter_by_range, filtered_slope,apply_xy_shift, apply_z_shift, mask_as_array, dilate_mask option, Coreg.residuals and error)

@adehecq adehecq mentioned this issue Oct 25, 2022
9 tasks
@rhugonnet
Copy link
Member Author

rhugonnet commented Jul 29, 2023

Adding some points after closing #158:
9. Move apply_matrix from base.py to affine.py by overridding apply() in AffineCoreg (as done in BiasCorr), and related functions,
10. Add a run or fit_and_apply class method for Coreg,
11. Add a is_rigid property for AffineCoreg and subclasses,
12. Improve tests of apply_matrix or add a geo-ref based one? This joins #110.
13. Refine documentation pages to differentiate affine/non-affine (bias corrections) and explain the generic inputs/ouputs.

@rhugonnet rhugonnet added the priority Needs to be fixed rapidly label Aug 1, 2023
@rhugonnet
Copy link
Member Author

Since #436, relevant for the re-org: Make subsampling consistent in AffineCoreg classes once the inside of _fit_func are consistently defined as functions working on 1D vector inputs (as in BiasCorr)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority Needs to be fixed rapidly
Projects
None yet
2 participants