bias_vars causing errors

[Elizabethcase]

Hi,

Excited to see the coregistration methods being continuously updated. I'm running into some errors right now with inconsistencies in how bias_vars is used/called. Just a note that it would also help to have a list of examples for what variables to use for bias_vars.

When I run xdem.coreg.Deramp(poly_order=2) on its own, it works fine When calling in a pipeline, e.g., coreg_pipeline2_bcnk = xdem.coreg.Deramp(poly_order=2) + xdem.coreg.NuthKaab(), I get an error:

File ~/mambaforge/envs/hsfm/lib/python3.10/site-packages/xdem/coreg/base.py:766, in Coreg.fit(self, reference_dem, dem_to_be_aligned, inlier_mask, transform, crs, weights, subsample, verbose, random_state, **kwargs)
    755 ref_dem, tba_dem, transform, crs = _preprocess_coreg_raster_input(
    756     reference_dem=reference_dem,
    757     dem_to_be_aligned=dem_to_be_aligned,
   (...)
    762     random_state=random_state,
    763 )
    765 # Run the associated fitting function
--> 766 self._fit_func(
    767     ref_dem=ref_dem,
    768     tba_dem=tba_dem,
    769     transform=transform,
    770     crs=crs,
    771     weights=weights,
    772     verbose=verbose,
    773     random_state=random_state,
    774     **kwargs,
    775 )
    777 # Flag that the fitting function has been called.
    778 self._fit_called = True

File ~/mambaforge/envs/hsfm/lib/python3.10/site-packages/xdem/coreg/base.py:1324, in CoregPipeline._fit_func(self, ref_dem, tba_dem, transform, crs, weights, verbose, **kwargs)
   1322 if verbose:
   1323     print(f"Running pipeline step: {i + 1} / {len(self.pipeline)}")
-> 1324 coreg._fit_func(ref_dem, tba_dem_mod, transform=transform, crs=crs, weights=weights, verbose=verbose)
   1325 coreg._fit_called = True
   1327 tba_dem_mod, out_transform = coreg.apply(tba_dem_mod, transform, crs)

TypeError: Deramp._fit_func() missing 1 required positional argument: 'bias_vars'

This is in v0.0.13

Thanks,
Elizabeth

[rhugonnet]

Hi @Elizabethcase,

Thanks for reporting this issue! This is likely because we've moved Deramp to the new non-affine coregs (or "bias corrections") classes: https://xdem.readthedocs.io/en/stable/biascorr.html. They should still be usable in a CoregPipeline object, like the one you create with the sum, but it is not thoroughly tested yet, just the basics. I'll have a look in the next days to fix that!

There will also be a restructuration of Coreg coming in the fall (and we'll aim for a stable xDEM release right after that, with proper deprecation warnings etc), so keep note of the current package versions you are using if you want to keep your old codes running (you'll always be able to install old xDEM versions 🙂).

[erikmannerfelt]

Hi,

Excited to see the coregistration methods being continuously updated. I'm running into some errors right now with inconsistencies in how bias_vars is used/called. Just a note that it would also help to have a list of examples for what variables to use for bias_vars.

When I run xdem.coreg.Deramp(poly_order=2) on its own, it works fine When calling in a pipeline, e.g., coreg_pipeline2_bcnk = xdem.coreg.Deramp(poly_order=2) + xdem.coreg.NuthKaab(), I get an error:

File ~/mambaforge/envs/hsfm/lib/python3.10/site-packages/xdem/coreg/base.py:766, in Coreg.fit(self, reference_dem, dem_to_be_aligned, inlier_mask, transform, crs, weights, subsample, verbose, random_state, **kwargs)
    755 ref_dem, tba_dem, transform, crs = _preprocess_coreg_raster_input(
    756     reference_dem=reference_dem,
    757     dem_to_be_aligned=dem_to_be_aligned,
   (...)
    762     random_state=random_state,
    763 )
    765 # Run the associated fitting function
--> 766 self._fit_func(
    767     ref_dem=ref_dem,
    768     tba_dem=tba_dem,
    769     transform=transform,
    770     crs=crs,
    771     weights=weights,
    772     verbose=verbose,
    773     random_state=random_state,
    774     **kwargs,
    775 )
    777 # Flag that the fitting function has been called.
    778 self._fit_called = True

File ~/mambaforge/envs/hsfm/lib/python3.10/site-packages/xdem/coreg/base.py:1324, in CoregPipeline._fit_func(self, ref_dem, tba_dem, transform, crs, weights, verbose, **kwargs)
   1322 if verbose:
   1323     print(f"Running pipeline step: {i + 1} / {len(self.pipeline)}")
-> 1324 coreg._fit_func(ref_dem, tba_dem_mod, transform=transform, crs=crs, weights=weights, verbose=verbose)
   1325 coreg._fit_called = True
   1327 tba_dem_mod, out_transform = coreg.apply(tba_dem_mod, transform, crs)

TypeError: Deramp._fit_func() missing 1 required positional argument: 'bias_vars'

This is in v0.0.13

Thanks, Elizabeth

Hi @Elizabethcase,

Great that we get to know this so we can fix it! In the meantime, you can run both approaches separately; the pipeline object is just a convenience tool to reduce the amount of code that's needed:

deramp = xdem.coreg.Deramp(poly_order=2)
deramp.fit(ref, tba)
deramped_tba = deramp.apply(tba)

nuthkaab = xdem.coreg.NuthKaab()
nuthkaab.fit(ref, deramped_tba)
final_tba = nuth_kaab.apply(deramped_tba)

This is more or less what the pipeline object does under the hood.

[adehecq]

Hi @Elizabethcase,
That's a good catch. I had the same issue today after updating my version of xdem.

@rhugonnet
An easy way to reproduce the issue is to run the Nuth & Kaab example and just replace the nuth_kaab instance with nuth_kaab = xdem.coreg.NuthKaab() + xdem.coreg.Deramp().
Actually, this will fail with any class of biascorr.py, e.g. xdem.coreg.BiasCorr().

I had a quick look to see if I could fix it, but this seem rather complex. All biascorr classes have an additional bias_var argument for their _fit_func but not the affine methods. This needs to be taken into account in the CoregPipeline loop here.

However, I also noticed that BiasCorr() behaves differently and does not have a _meta["bias_vars"] item. Therefore, calling BiasCorr().fit will also not work as expected. To reproduce, you can run the Deramp example but replace the normal Deramp instance with deramp = xdem.coreg.BiasCorr(). This will fail.
I was using the existence of this item to test whether or not the instance was an affine method or not. Btw, is there an easy way to discriminate affine vs non-affine methods? There is the _is_affine attribute, but e.g. for NuthKaab this is False.

[adehecq]

I also noticed that the attribute is_affine (without the preceding underscore) is called twice in core/base.py, while it does not exist...

[rhugonnet]

Yes it's a bit complex, probably easiest if I dive into this (familiarity with BiasCorr).

Busy until Friday, should be able to have a look then.

[rhugonnet]

Looks like the fix is fairly straightforward! 😄 Adding a None default to methods that run without a bias_vars argument, and using keyword args when calling CoregPipeline._fit_func and ._apply_func methods to not rely on positional: https://github.com/GlacioHack/xdem/pull/424/files#diff-aea2d63525f9a40a7ff63d3ff77e9debd0d30dcb8065ca6765620110e403edb3R794

Still, this also raises the issue of how to make a pipeline with BiasCorr method that do require a bias_vars. I'll look at adding that tomorrow, and I am using the opportunity to restructure tests a bit and add several more!

[erikmannerfelt]

Looks like the fix is fairly straightforward! 😄 Adding a None default to methods that run without a bias_vars argument, and using keyword args when calling CoregPipeline._fit_func and ._apply_func methods to not rely on positional: https://github.com/GlacioHack/xdem/pull/424/files#diff-aea2d63525f9a40a7ff63d3ff77e9debd0d30dcb8065ca6765620110e403edb3R794

Still, this also raises the issue of how to make a pipeline with BiasCorr method that do require a bias_vars. I'll look at adding that tomorrow, and I am using the opportunity to restructure tests a bit and add several more!

Hmm, I see two solutions:

1. Allow the bias_vars to be set on instantiation as well as fit:

>>> coreg = xdem.coreg.NuthKaab() + xdem.coreg.Deramp(bias_vars=...)
>>> coreg.fit(...)

2. Allow arbitrary **kwargs to be set when running fit (which would then allow passing bias_vars to all pipeline steps)

I see advantages and disadvantages of both. In 1), different values can be entered for chained coregs/filters. The disadvantage is that .fit() would only work on the same shape/transform as the given bias_vars. A bit ugly, but perhaps not the end of the world.

In 2), bias_vars could be propagated to all underlying steps in the pipeline, for better or worse. I guess that could be nice in terms of brevity, but could also have unforeseen consequences!

[rhugonnet]

Yes, it's not trivial... 😅

I don't like 1) so much because I think we don't wanna store anything else than metadata during a Coreg class instantiation, or it would create a lot of new problems to deal with. One solution would be to pass only the names:

pipeline = xdem.coreg.NuthKaab() + xdem.coreg.Bias1D(bias_var_names=["slope"]) + xdem.coreg.Bias2D(bias_var_names=["ncc", "slope"])

which would then be recognized in the dictionary bias_vars:{str: Raster | ndarray} passed to Coreg.fit() and .apply() to be parsed properly.

Alternatively, 2) could work implicitly in many cases: the length of the bias_vars dictionary could be used to check that it matches the number of bias_vars expected by the pipeline. Re-using the example above, we could detect there are 3 variables expected from Bias1D + Bias2D, and they would be parsed in order...

pipeline.fit(bias_vars={"slope": slp, "ncc": ncc, "slope": slp})

but, as shown above, it could involve duplicating some variables like slope... And BiasND does not explicitly defines a number of bias_vars expected.

I'm thinking to use the best of both 1) and 2)?
We allow the definition of bias_var_names as in 1) which allows fully explicit definition. But, simultaneously, we set bias_var_names as optional None by default, in case implicit behaviour can be logically derived using 2). If not, and a bias_var_names was not passed, we raise a UserWarning that the variable used might not be the right one (or maybe even an error)?