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

Fix CoregPipeline to work with biascorr methods #424

Merged
merged 8 commits into from
Sep 6, 2023

Conversation

rhugonnet
Copy link
Contributor

@rhugonnet rhugonnet commented Sep 1, 2023

This PR makes CoregPipeline work with BiasCorr classes, and makes Coreg's behaviour consistent with bias_vars at all levels.
It also renames coreg/pipelines.py into coreg/workflows.py to avoid confusion.

In short, regarding core functionalities, this PR:

  • Adds the bias_vars argument directly into Coreg.fit() and .apply(), and all subclasses to match signature (even if never used in AffineCoreg, checked directly in Coreg.fit() and .apply() and raises a warning if a bias_vars was passed for nothing). This resulted in the removal of BiasCorr.fit() and BiasCorr.apply() that were originally overridding Coreg to define bias_vars.
  • Renames the bias variable names Coreg._meta["bias_vars"] (that used to be stored during .fit()) into Coreg._meta["bias_var_names"] which can be optionally defined directly at instantiation, e.g. BiasCorr1D(bias_var_names=["slope"]). This allows to explicitly define which variables bias_vars (that are named because passed as a dict) are associated to each bias-correction method in a pipeline. For other cases, it is not necessary and can just use what is passed to fit(). If it is defined, however, the two inputs need to be consistent (or raises an error).
  • Adds a Coreg class attribute _needs_vars to differentiate between coregistration methods that need bias variables to run (e.g., BiasCorr1D) and those who don't (e.g., NuthKaab or Deramp, even though one is affine and the other non-affine).
  • Adds a CoregPipeline._parse_bias_vars() method to parse the bias variables needed for a pipeline step (works for both fit and apply), or raise appropriate errors.

For tests, the PR:

  • Restructures the coregistration tests mirroring the structure of xdem/coreg to sort the big mess!
  • Adds several pipeline tests to check all combinations of existing Coreg subclasses can be combined, including those requiring a bias_vars,
  • Adds several tests to check clear errors are appropriately raised for cases where bias_vars is incorrectly defined (in a pipeline or in a single biascorr class),
  • Adds some other small tests to ensure new class attributes are properly set.

Resolves #413
Resolves #405

Opened #427

@erikmannerfelt
Copy link
Contributor

Awesome! Great that you're splitting up the regular coreg and the pipeline/blockwise test cases into multiple classes. That makes it much cleaner! I wonder if they should maybe be in different files, potentially mirroring the coreg submodule structure. I mean, coreg is getting quite big:

  • Base coreg functionality (including pipelines)
  • Coreg workflows
  • Blockwise
  • Affine co-registration (ICP, NuthKaab, GradientDescending)
  • All the different types of bias correction
  • Filters in the future

Maybe it would be better to have multiple test files to make it less of a monster?

@rhugonnet
Copy link
Contributor Author

Agreed! I'll fully mirror the folder structure of xdem/ for consistency.

@rhugonnet
Copy link
Contributor Author

rhugonnet commented Sep 2, 2023

Alright, all done following #413 (comment)!
I'll add comments to the PR where things are new (hard to see what changed with the restructuration of tests), and update the PR description! 😄

# Assert that the combined vertical shift is 2
assert pipeline2.to_matrix()[2, 3] == 2.0

all_coregs = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New from here to test_pipeline_pts

Copy link
Member

Choose a reason for hiding this comment

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

Looking good !

@@ -723,13 +711,28 @@ def __add__(self, other: CoregType) -> CoregPipeline:
raise ValueError(f"Incompatible add type: {type(other)}. Expected 'Coreg' subclass")
return CoregPipeline([self, other])

@property
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was in the wrong spot (needs to be at Coreg level I think, not Affine/NonAffine!).

@@ -1316,13 +1359,55 @@ def copy(self: CoregType) -> CoregType:

return new_coreg

def _parse_bias_vars(self, step: int, bias_vars: dict[str, NDArrayf] | None) -> dict[str, NDArrayf]:
Copy link
Contributor Author

@rhugonnet rhugonnet Sep 2, 2023

Choose a reason for hiding this comment

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

The non-public CoregPipeline method that makes the pipeline work smoothly for both fit and apply

}
super().__init__(meta=meta_bin_and_fit) # type: ignore

# Update attributes
self._fit_or_bin = fit_or_bin
self._is_affine = False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used to have these two here to override Coreg.fit() and .apply() (and not only the ._fit_func(), ._apply_func()), but now that bias_vars has been moved to Coreg directly, there is no need for them anymore.

@rhugonnet
Copy link
Contributor Author

Tests all passing, the current error comes from SciPy: scipy/scipy#19103
We need to wait until they release 1.11.3 to get the fix.

Copy link
Member

@adehecq adehecq left a comment

Choose a reason for hiding this comment

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

I didn't manage to look at the tests yet. I will try to do tomorrow. But the changes in the code itself seems alright.
Well done ! 🙌

@rhugonnet rhugonnet merged commit ed3a7fe into GlacioHack:main Sep 6, 2023
11 checks passed
@rhugonnet rhugonnet deleted the fix_biascorr_pipelines branch September 6, 2023 00:10
@adehecq
Copy link
Member

adehecq commented Sep 6, 2023

I just had a look at the tests (as you said not easy to spot the actual changes with all the shuffling around, thanks for pointing to the most important part).
It's all more consistent now and the tests have improved in coverage. Thanks !

@adehecq
Copy link
Member

adehecq commented Sep 6, 2023

Maybe a minor comment. Should we expand the docstrings at the beginning of each coreg submodule to better explain what it contains?
E.g in base.py: """Base coregistration classes to define generic methods and pre/post-processing of input data.""" -> add a short description of the base Coreg class and attributes, the Pipeline and the Blockwise? Or is it too redundant with the doc and or the specific docstrings?

@rhugonnet
Copy link
Contributor Author

Maybe a minor comment. Should we expand the docstrings at the beginning of each coreg submodule to better explain what it contains? E.g in base.py: """Base coregistration classes to define generic methods and pre/post-processing of input data.""" -> add a short description of the base Coreg class and attributes, the Pipeline and the Blockwise? Or is it too redundant with the doc and or the specific docstrings?

Yes, we need to do this! Opened #434

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.

bias_vars causing errors Could coreg/pipelines.py get a different name?
3 participants