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 Coregistration Functionality for CLI #629

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

vschaffn
Copy link
Contributor

@vschaffn vschaffn commented Oct 31, 2024

Resolves #622 and #623

Description

This PR implements the coregistration functionality for the CLI. The aim is to enhance the xdem run by adding the necessary functions to verify the existence of both input paths and executing the coregistration of DEMs.

Key Changes:

  1. Path Verification:

    • Implemented functionality to verify that the paths for both the reference and secondary DEMs exist before processing.
  2. Loading DEMs:

    • Utilized the geoutils.rasters.load_multiple_rasters accessor to load the reference and secondary DEMs:
      reference_dem, to_be_aligned_dem = geoutils.raster.load_multiple_rasters([ref_dem_path, tba_dem_path])
  3. Coregistration Execution:

    • Integrated the dem_coregistration function from the workflows module to perform the coregistration:

      coreg_dem, coreg_method, out_stats, inlier_mask = dem_coregistration(
         to_be_aligned_dem, reference_dem, "aligned_dem.tiff"
      )
    • The aligned DEM is saved as aligned_dem.tiff, and the inlier mask is saved as inlier_mask.tiff.

  4. Output Verification:

    • Added print statements to output the metadata of the coregistered DEM and the resulting statistics.

Testing:

Developed tests to retrieve test data and validate the outputs against ground truth data as specified in tests/test_cli.py.

Documentation

Updated the Quick Start Guide in the documentation to reflect the new coregistration capabilities and usage instructions.

Future Work:

As #617 is not merged yet, the changes made do not take account of the change of licence. After #617 merge, this PR should be update with a header for xdem_cli.py and a description of the package argcomplete in the NOTICE file.

@rhugonnet
Copy link
Contributor

Great! 🙂

I don't have much to comment on the code, my main remark at this stage is rather on the structure: Should we already anticipate other CLI usage than coregistration (e.g., terrain attributes, uncertainty)?

If yes, we could potentially separate into different namings, for instance by adding a first parameter to the parsing: xdem coregister path_ref path_sec, which could be extended to other modules to run for instance xdem terrain "slope", xdem uncertainty "method1" etc.
But this is a structure that works only if the functionalities are independent. We could also think of another structure where all these steps can be run sequentially (for instance all users run the uncertainty after coregistration), and everything is combined into a single call like xdem pipeline *params.
Or we could have both.
The structure(s) will have to be reflected in the JSON config file(s), so worth thinking ahead I think!

Small remarks:

  • Should we modify "path_sec" -> "path_tba" to match the Python API? Or we can change the Python API.
  • To save inlier_mask, can also write more shortly (duplicates the boolean data, which is not ideal even if it's pretty light; we'll have the _save() function callable on the NumPy array in GeoUtils to avoid that soon):
inlier_rst = coreg_dem.copy(new_array=inlier_mask)
inlier_rst.save("xxx.tif")

@vschaffn
Copy link
Contributor Author

vschaffn commented Nov 5, 2024

Thanks for your feedback @rhugonnet.

  • I agree with you for the anticipation of other CLI usages, then I have restructured the code to allow other uses to be integrated.
    Now, to perform coregistration, the command is :
    xdem [options] coregister path_ref path_tba

  • I have changed the path name in the documentation to match with Python API

  • Thanks for the tip to save the inlier_mask in a simpler way, even if it is not perfect 👍

@vschaffn vschaffn force-pushed the 623-simple_cli_coreg branch 2 times, most recently from f050c91 to 45fcd54 Compare November 12, 2024 09:28
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.

[POC] simple CLI
2 participants