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

Implement one-liner coregistration #267

Merged
merged 14 commits into from
Oct 18, 2022
Merged

Implement one-liner coregistration #267

merged 14 commits into from
Oct 18, 2022

Conversation

adehecq
Copy link
Member

@adehecq adehecq commented May 12, 2022

This is mainly to fix issue #257.
See David and old own implementations for ideas.

This PR adds a function in coreg.py to run all the coregistration steps in one line, i.e.:

  • load the datasets and reproject on the same grid
  • create the inlier mask by excluding glaciers, steep slopes and large blunders
  • estimate the coregistration: setup so that it can be split into a separate horizontal alignment (with method set by string, default is 'nuth_kaab') and a vertical alignment (default is 'median', i.e remove a median bias).
  • apply the coregistration
  • optionally plot a figure
  • calculate the statistics of before and after coreg
  • returns the coregistered DEM (optionally save to file) and statistics

To be added:

  • add tests
  • allow inputs to be DEM instance rather than path to file

@adehecq adehecq marked this pull request as draft May 12, 2022 15:10
@adehecq adehecq linked an issue Oct 6, 2022 that may be closed by this pull request
@adehecq adehecq marked this pull request as ready for review October 18, 2022 07:58
@adehecq
Copy link
Member Author

adehecq commented Oct 18, 2022

Bypassing tests since it is an opened issue addressed in PR #322.
Bypassing reviews to merge quickly, but @rhugonnet you can comment a posteriori since I will open a new PR to continue this later.

@adehecq adehecq merged commit 792a486 into GlacioHack:main Oct 18, 2022
@adehecq adehecq mentioned this pull request Oct 18, 2022
Copy link
Contributor

@rhugonnet rhugonnet left a comment

Choose a reason for hiding this comment

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

Nice to have a first version of this done! 😄

We probably don't want to spend to much time improving it right now, because the Coreg classes will change and so will the code of this function.

In more details (not much can be done right now, just to keep track for the future), we would probably need to include:

  • The filtering argument with the possibility to call classes of filters.py that could be combined with Coreg objects,
  • Same for biascorr.py classes,
  • Extract the statistics directly computed within the Coreg classes in ._metadata (that needs to become a user-available attribute) to stay fully consistent with the results of the classes, instead of re-performing them manually here,
  • Use the plots directly generated within the Coreg classes once those exist,
  • Have an option for co-registration with point data once this exists,
  • Use Coreg.pipeline() instead of performing the steps manually.

And, inversely, some parameters of this function should probably be moved to Coreg class so that they retain all flexibility:

  • The grid argument should also be an option of Coreg objects, that always performs the same operation by default right now,
  • Adding exclude_mask, include_mask might also be a great option, instead of just having an inlier_mask that requires projecting vectors on the same grid, but the mask passed should retain full flexibility (Raster object, Vector objects, or both).

In short, eventually, I think the one-liner co-registration needs to become a convenience function that passes the most robust default parameters to a CoregPipeline object and captures the outputs. All its options have to be available directly in the Coreg classes, while maintaining complete flexibility.
Do you picture it the same, @adehecq?

xdem/coreg.py Show resolved Hide resolved
:param src_dem_path: path to the input DEM to be coregistered
:param ref_dem: 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 shpfile: path to a vector file containing areas to be masked for coregistration
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably use filename_vector for clarity with the parameter naming of GeoUtils functions, or maybe something similar than this function https://github.com/GlacioHack/xdem/blob/main/xdem/spatialstats.py#L445 for flexibility on inclusion or exclusion of the shapefile.

:param vmode: The method to be used for vertically aligning the DEMs, e.g. mean/median bias correction or \
deramping. Can be any of {list(hmodes_dict.keys())}.
:param deramp_degree: The degree of the polynomial for deramping.
:param grid: the grid to be used during coregistration, set either to "ref" or "src".
Copy link
Contributor

Choose a reason for hiding this comment

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

The name grid is not so transparent as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would you use?

@adehecq
Copy link
Member Author

adehecq commented Oct 21, 2022

In more details (not much can be done right now, just to keep track for the future), we would probably need to include:

* The `filtering` argument with the possibility to call classes of `filters.py` that could be combined with `Coreg` objects,

* Same for `biascorr.py` classes,

Good ideas yes.

* Extract the statistics directly computed within the `Coreg` classes in `._metadata` (that needs to become a user-available attribute) to stay fully consistent with the results of the classes, instead of re-performing them manually here,

* Use the plots directly generated within the `Coreg` classes once those exist,

* Have an option for co-registration with point data once this exists,

Yes, yes and yes.

* Use `Coreg.pipeline()` instead of performing the steps manually.

There is an option to provide a Coreg.pipeline directly to coreg_method. The idea behind splitting the horizontal vs vertical alignment is that one might need to filter pixels differently (use a different inlier mask) for these two steps. I'm thinking for example about flat areas, that are not useful for Nuth & Kaab, but very useful for BiasCorr.

And, inversely, some parameters of this function should probably be moved to Coreg class so that they retain all flexibility:

* The `grid` argument should also be an option of `Coreg` objects, that always performs the same operation by default right now,

* Adding `exclude_mask`, `include_mask` might also be a great option, instead of just having an `inlier_mask` that requires projecting vectors on the same grid, but the mask passed should retain full flexibility (`Raster` object, `Vector` objects, or both).

Indeed, we could make the reprojection and creation of the inlier_mask part of the Coreg tools directly...

In short, eventually, I think the one-liner co-registration needs to become a convenience function that passes the most robust default parameters to a CoregPipeline object and captures the outputs. All its options have to be available directly in the Coreg classes, while maintaining complete flexibility. Do you picture it the same, @adehecq?

Yes, that seems sensible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

coreg.apply creates artifacts on the edges
2 participants