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 4 : adopt every coregistration method #556

Open
2 tasks
adebardo opened this issue Aug 1, 2024 · 5 comments
Open
2 tasks

[POC] cli 4 : adopt every coregistration method #556

adebardo opened this issue Aug 1, 2024 · 5 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 all affine coregistration methods specified in the configuration file.

The available methods are:

  • ICP
  • VerticalShift
  • GradientDescending
  • Nuthkaab
  • Tilt

Code Changes

In the run function, we need to allow the use of different coregistration methods:

  • Modify class pydantic_Coregistration(BaseModel) to accept all coregistration methods.

  • Modify the run function as follows:

    coreg_methods = {  
        "NuthKaab": coreg.NuthKaab,  
        "ICP": coreg.ICP, 
        ... 
    }  
   coreg_dem, _, _, _ = dem_coregistration(reference_elev, to_be_aligned_elev, config["outputs"]["path"], coreg_methods[config["coregistration"]["method"]])

Allowing pipelines

The user can combine several coregistration methods, so we need to allow them to add up to 3 methods.

[coreg]
method1 = "NuthKaab"
method2 = "Deramp"

Allowing parameters

Coregistration methods can be configured, and we propose storing the parameters in a dictionary, which will then be validated using Pydantic (following the provided example for inspiration).

[coreg]
method1 = "NuthKaab"
method1_args = {"subsample": 100000}

method2 = "Deramp"
method2_args = {"poly_order": 1}

exemple code :

# Définir un modèle Pydantic
class CoregParam(BaseModel):
    subsample: int
    poly_order: int

data_invalid = {
    "subsample":100000,
    "poly_order": "one",  # error here, "poly_order" must be an int 
}

try:
    parameters_config = CoregParam(**data_invalid)
except ValidationError as e:
    print("Validation error:", e.json())

Tests

In the tests/test_cli file, create tests for all coregistration functions and check their outputs. You can use the tests available at https://github.com/GlacioHack/xdem/tree/main/tests/test_coreg for inspiration.

Documentation

Update the CLI documentation to reflect the new coregistration methods. This should include:

  • A list of available coregistration methods.
  • Instructions on how to specify the coregistration method in the configuration file.
  • Examples showing how to use each coregistration method in the configuration.

Estimation

5d

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

see remarks in #554 on pipeline code organization and a factory to add every coregistration method and limit code duplication.

@adehecq
Copy link
Member

adehecq commented Sep 17, 2024

I think what needs to be discussed is how we write the config file to support pipelines. In the API, a pipeline is a sum (with "+" symbol) of coreg methods, each of which have some optional arguments. For example: coreg.NuthKaab(subsample=100000) + coreg.Deramp(poly_order=1).

A simple option could be to accept a string listing exactly this, but I don't think that's the idea behind the CLI? In that case, we need to find a different way to describe the pipeline. Luckily, as pointed out by @rhugonnet, since PR #530, all input parameters of the coreg methods have the same names. One option could be:

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

[outputs]
path = "path/to/save/folder" 

[coreg]
method1 = "NuthKaab"
method1_args = {"subsample": 100000} 

method2 = "Deramp"
method2_args = {"poly_order": 1} 

The number of allowed coreg methods could be set to any between 0 and 3?
@rhugonnet, would that be sufficient to list all arguments at once, or would we have to split for different kind of arguments, like random_args, fitting_or_binning_args, iterative_args etc?

@rhugonnet
Copy link
Member

Yes, if we can write a dictionary of arguments like you did above, this is sufficient to list all at once (no need to split them by type).
If we need to enumerate them below the method, it will start to be a bit long... but should still be OK as most users will only want to tweak two or three (like subsample, tolerance, max_iterations, and potentially another one specific to the method).

The arguments would be exactly the same as exposed through the __init__ of the Coreg class, so it would be easy for users to refer to the API. And arguments common to all methods (randomization, iterative, affine and fitting/binning) would also be listed in the doc, like this tentative section in the ongoing doc PR: https://xdem-rhugonnet.readthedocs.io/en/towards_0.1/coregistration.html#accessing-coregistration-metadata.

Additionally, some arguments might be completely unusable through the CLI, like fit_optimizer that requires to pass a new callable doing the fit like scipy.optimize.curve_fit. If we wanted to allow some, I guess we'd have to map pre-defined optimizer wrappers (like the current sklearn ones stored in fit.py) from a string, but that would be it!

@adebardo
Copy link
Author

adebardo commented Sep 18, 2024

Hello to both of you, I was planning to open a special ticket for the pipelines and parameters settings, but I can try to handle it here instead; I'll just add two extra day of development.

I note the suggestion to use a dictionary, though we'll need to see if it complicates things on the development side when it comes to validation.

I propose opening a ticket for the problematic arguments.

@adebardo
Copy link
Author

@adehecq Is there a method similar to ** dem_coregistration** for replacement coreg.NuthKaab(subsample=100000) + coreg.Deramp(poly_order=1) ?

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