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.

@rhugonnet
Copy link
Member

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 vschaffn force-pushed the 623-simple_cli_coreg branch from 8f53ba9 to 42c96dc Compare November 5, 2024 10:55
@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
@vschaffn vschaffn force-pushed the 623-simple_cli_coreg branch 4 times, most recently from c72d5be to dba0af8 Compare November 21, 2024 09:44
@vschaffn vschaffn force-pushed the 623-simple_cli_coreg branch 2 times, most recently from 6602bab to b9af0e1 Compare November 22, 2024 13:50
@vschaffn
Copy link
Contributor Author

Updated following the merge of PR #617

@vschaffn vschaffn force-pushed the 623-simple_cli_coreg branch 2 times, most recently from 2257f39 to 8b2b856 Compare November 27, 2024 11:15
@vschaffn vschaffn force-pushed the 623-simple_cli_coreg branch from 8b2b856 to e0b9b97 Compare December 4, 2024 10:44
@vschaffn vschaffn force-pushed the 623-simple_cli_coreg branch 2 times, most recently from 0c4313d to 3940dc1 Compare December 12, 2024 10:11
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