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

[POC] cli step 3 add coreg nuth&kaab #554

Open
7 tasks
adebardo opened this issue Aug 1, 2024 · 6 comments
Open
7 tasks

[POC] cli step 3 add coreg nuth&kaab #554

adebardo opened this issue Aug 1, 2024 · 6 comments
Labels
[POC] Conception To review Tickets needs approval about it conception

Comments

@adebardo
Copy link

adebardo commented Aug 1, 2024

Context

The goal of this ticket is to implement coregistration from the xdem run.

In the code

From the run in the init file, we now need to:

[inputs]
reference_elev = "path/to_example.tif"
to_be_aligned_elev = "path/to_example.tif"

[outputs]
path = "path/to/save/folder"
  • add the right function to veirfy path, if "path/to/save/folder" doesn't exist, it can be created

  • Load the reference and secondary DEMs using the accessor:
    reference_elev, to_be_aligned_elev = geoutils.rasters.load_multiple_rasters([config["inputs"]["reference_elev"], config["inputs"]["to_be_aligned_elev"])

  • Execute a coregistration:

from worflows import dem_coregistration

coreg_dem, _, _, _ = dem_coregistration(reference_elev, to_be_aligned_elev, config["outputs"]["path"])

For the tests

  • Retrieve test data and ground truth data
  • Verify that the outputs matches the ground truth from here in the file tests/test_cli.py
  • add tests for new entry

For the documentation

  • Update the quick start guide in the documentation

2d

@adebardo adebardo added the [POC] Conception To review Tickets needs approval about it conception label Aug 1, 2024
@duboise-cnes
Copy link
Member

remarks:

    # Create coregistration object
    coregistration_ = Coregistration(cfg) # --> could be Coregistration(name_of_coreg "nuth_kaab", ...) not the full config 
    # Compute coregistration
    _ = coregistration_.compute_coregistration(input_sec, input_ref)

--> for glaciohack, conceptually, we have separated in demcompare way : a Coregistration Class which is a simple factory of CoregistrationTemplate objects depending on a parameter (config, string...). This CoregistrationTemplate is the equivalent of your Coreg class which is the class containing all of common elements and that drive API for all (usually using ABCMeta, abstractmethod but i don't know the equivalent with @overload you seem to use, https://github.com/GlacioHack/xdem/blob/main/xdem/coreg/base.py#L1215).
So for a pipeline and a cli this must be solved and validated, i think. To be discussed.

Maybe in two steps, a first basic way as here to make a first cli and another adding a more flexible code organization.
Seems not so difficult to add a factory in coreg of xdem. Don't know the potential impact on an API for now.

My 2 cts, hoping it helps

@adehecq
Copy link
Member

adehecq commented Aug 9, 2024

A few comments:

  • to open the data, the best is to use geoutils.rasters.load_multiple_rasters which optmize memory usage by first downloading only metadata, cropping and reprojecting only in areas of overlap between the two DEMs.
  • I see you only ran the fit method, but you also need to run apply to get the coregistered DEM
  • you can find all this and an example of a typical workflow in the function dem_coregistration.

And to answer @duboise-cnes's questions:

  • @rhugonnet may contradict me, but I don't think we have an easy way to list all coreg methods. It would certainly be useful though
  • I think you're searching for function the function dem_coregistration (which I mentioned just above)?
  • I think you misunderstood the "@overload"? It's used to make mypy happy when outputs can have different types depending on the input arguments. Or would you mind better explaining the issue?

@rhugonnet
Copy link
Contributor

For listing coreg methods: I think it would be easy to maintain a manual list, as we don't add a new method very often (there are list already used in test_biascorr, test_coreg/test_base and soon in test_affine, we could make them consistent by adding an available dictionary in each coreg sub-module as @duboise-cnes mentions). From the list we could automatically parse the different arguments of each coregistration class.

Note: Regarding the parameters, after the merge of PR #530 (that I'm working on this week), all Affine coregistration methods will have exactly the same arguments across the different categories:

  • Parameters of iterative algorithm if any (tolerance or max iterations where to stop),
  • Parameters of randomization (subsample sizes, random state for subsampling and some optimizers),
  • Parameters of optimization (fitting and binning): fitting function, optimizer, bin sizes, binning statistics, etc.

This is about to be documented (once #502 is finalized, hopefully by the time you are all back from holidays 😉).

@adebardo
Copy link
Author

@duboise-cnes thanks, i think we could talk about the coregistration architecture in a meeting
@adehecq Thanks for the tip. For the fit method, I would prefer to discuss the type of outputs you want and how we want to save things.
@rhugonnet Great news for the parameters :)

I plan to revisit this matter after the 21/08/2024 meeting.

@adehecq
Copy link
Member

adehecq commented Sep 17, 2024

I plan to revisit this matter after the 21/08/2024 meeting.

Is there anything new to discuss here?

@adebardo
Copy link
Author

I plan to revisit this matter after the 21/08/2024 meeting.

Is there anything new to discuss here?

I already changed the ticket regarding the meeting yes, it's okay now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[POC] Conception To review Tickets needs approval about it conception
Projects
None yet
Development

No branches or pull requests

4 participants