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

Select resampling algo #141

Merged
merged 4 commits into from
Apr 25, 2021
Merged

Conversation

spadarian
Copy link
Contributor

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This PR changes the functions used to read rasters by adding an extra parameter to select the resampling algorithm used when buffer_size != window size (GDAL needs to interpolate the values) (closes #140)

@spadarian
Copy link
Contributor Author

I've just started using Rust, so I'm not sure this is the best implementation (it introduces some breaking changes).

I'm happy to change it if there is a better way!

@lnicola
Copy link
Member

lnicola commented Mar 2, 2021

Looks like this needs rebasing (Dataset moved IIRC). I'm not terribly familiar to GDAL, does this API support reading a window and resampling at the same time?

@spadarian
Copy link
Contributor Author

It does. When reading a window, if window_size != buffer_size then GDAL performs an interpolation. By default, it uses the nearest neighbour. This PR allows you to set a different resampling method.

@spadarian spadarian force-pushed the select_resampling_algo branch from 019bf04 to 565d241 Compare March 3, 2021 01:23
#[derive(Debug)]
pub struct RasterIOExtraArg {
pub n_version: libc::c_int,
pub e_resample_alg: GDALRIOResampleAlg::Type,
Copy link
Member

Choose a reason for hiding this comment

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

i think we should have an enum for the resample algorithms. That will make it easier for most users.

Copy link
Member

Choose a reason for hiding this comment

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

I think I actually prefer constants here because of the enum exhaustiveness matching issue. See also the layer caps PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GDALRIOResampleAlg::Type are already constants. Should I create a mod inside rasterband to map it?

Copy link
Member

@jdroenner jdroenner Mar 10, 2021

Choose a reason for hiding this comment

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

@lnicola i don't think that gdal will add more algorithms for resampling and they are an enum in GDAL anyway. Without an enum you can input "9323411". Or is there another issue with the enum approach?
@spadarian i would suggest to add an enum in rasterband.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jdroenner I added an enum with the names of the algorithms and a function to map them to integers (GDAL uses integers).

pub n_version: libc::c_int,
pub e_resample_alg: GDALRIOResampleAlg::Type,
pub pfn_progress: gdal_sys::GDALProgressFunc,
pub p_progress_data: *mut libc::c_void,
Copy link
Member

Choose a reason for hiding this comment

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

We should hide the pointer stuff for now. In the long run we could add a callback and/or a future/channel to access the state.

pub e_resample_alg: GDALRIOResampleAlg::Type,
pub pfn_progress: gdal_sys::GDALProgressFunc,
pub p_progress_data: *mut libc::c_void,
pub b_floating_point_window_validity: libc::c_int,
Copy link
Member

Choose a reason for hiding this comment

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

can we change this to i32 or another integer from core?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess an usize should be ok as well.

Just out of curiosity, what would be the difference? Just a "readability" thing? Or is there any technical reason?

Copy link
Member

Choose a reason for hiding this comment

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

just readability

@spadarian spadarian force-pushed the select_resampling_algo branch from 00365ac to 969a187 Compare March 23, 2021 23:18
@spadarian
Copy link
Contributor Author

I added the suggested changes and did a rebase.

@jdroenner jdroenner mentioned this pull request Apr 25, 2021
@jdroenner jdroenner merged commit c0babcd into georust:master Apr 25, 2021
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.

Any way of selecting GDALRIOResampleAlg?
3 participants