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

Allow custom subsampling for each CoregPipeline step separately. #137

Closed
erikmannerfelt opened this issue Jun 1, 2021 · 7 comments · Fixed by #141 or #436
Closed

Allow custom subsampling for each CoregPipeline step separately. #137

erikmannerfelt opened this issue Jun 1, 2021 · 7 comments · Fixed by #141 or #436
Assignees
Labels
enhancement Feature improvement or request

Comments

@erikmannerfelt
Copy link
Contributor

As mentioned by @adehecq in #135, Deramp is considerably slower than NuthKaab, so subsampling is preferred. However, subsampling does not necessarily need to be done on the NuthKaab step, so another argument was added. This should be possible in another way, and below is an example:

Each Coreg approach gets a subsample (or similar) optional argument which is empty by default. self.fit() can now accept a subsample argument which may be a string that says e.g. "custom", whereby it searches for the subsampling attribute within self. This means that one can do something along:

pipeline = xdem.coreg.NuthKaab(subsample=1) + xdem.coreg.Deramp(subsample=0.5)

pipeline.fit(..., subsample="custom")

Now, NuthKaab will run at full resolution, and Deramp will run at half.

It could also be that subsample for self.fit should be None by default (which in turn would mean 1.0, unless specified upon instantiation). Then it would just be:

pipeline = xdem.coreg.NuthKaab(subsample=1) + xdem.coreg.Deramp(subsample=0.5)

pipeline.fit(...)

The advantage of this approach over the argument added in #135 is that it would work on all Coreg subclasses; not just Deramp.

@adehecq
Copy link
Member

adehecq commented Jun 1, 2021

I think an easy fix for #135 would be to rename "max_points" by subsample.
The question is whether subsample should be the maximum number of points or the fraction of the total points. I would be in favor of the first option, as the fraction will highly depend on the input raster size and it would be difficult to set a good default for each method.
We agree that the use of the coreg methods should be as straightforward as possible and so the user shouldn't have to think about downsampling the data, except for very specific use cases. The current methods are still too slow in my opinion for medium size DEMs like 5000 x 5000, so a default subsample fraction of 1 does not seem appropriate in those cases.
For the Deramp example, I would be keen on keeping it the way it is, i.e. a default maximum number of points. Whatever the raster size, with a certain number of points we can easily constrain a degree 1 or 2 polynomial (the current set value of 5e5 is probably conservative). To make it more robust, we could possibly average the values over a certain radius.

@erikmannerfelt
Copy link
Contributor Author

Currently, the Coreg subsample argument parses it as a fraction if it's <= 1.0 and a count if it's > 1.0. I think that is pretty intuitive (when also properly documented, of course!)

The Deramp could have a subsample=0.5 or something as default if you think that it should.

In many cases, including ICP in many programs, subsampling is on by default. So it having subsampling by default is not untrodden ground.

@rhugonnet
Copy link
Contributor

Good to know for the fraction/count, that's quite useful (if well document, indeed!)

@adehecq adehecq self-assigned this Jun 3, 2021
@adehecq
Copy link
Member

adehecq commented Jun 3, 2021

I will work on it.

@adehecq adehecq linked a pull request Jun 3, 2021 that will close this issue
@erikmannerfelt
Copy link
Contributor Author

This is not solved by #141!

@adehecq
Copy link
Member

adehecq commented Jun 22, 2021

This is not solved by #141!

True!

@rhugonnet
Copy link
Contributor

rhugonnet commented Nov 28, 2023

This is now implemented by #436 (forgot to link that old issue)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature improvement or request
Projects
None yet
3 participants