georust / gdal

Rust bindings for GDAL
https://crates.io/crates/gdal
MIT License
339 stars 92 forks source link

Add histogram calculation for RasterBand #468

Closed spadarian closed 7 months ago

spadarian commented 8 months ago

This PR exposes 2 GDAL methods to calculate band histograms (default and user-defined).

spadarian commented 7 months ago

I've incorporated some of the changes suggested by @metasim, including:

I did not merge the calls for GDALGetDefaultHistogram and GDALGetRasterHistogram since I think having a API more or less consistent with the C code is desirable. Open to change it if that is where the project is going @lnicola @jdroenner

metasim commented 7 months ago

@spadarian Take a look at this PR against your repo for an alternative naming and associated documentation, which I think clarifies things for a future contributor.

The other option (assuming VSIFree is required instead of using drop), is to have default_histogram copy the allocated array into the same Vec as used by histogram, then VSIFree the array. I personally like this approach, but I would expect some to balk at the extra copy. Makes no difference to the user tho.

lnicola commented 7 months ago

We should also add set_default_histogram, but it's fine to do it in another PR.

spadarian commented 7 months ago

I addressed all the comments... but CI failed on a unrelated test ( raster::processing::dem::slope::tests::test_slope)

lnicola commented 7 months ago

Error: CplError { class: 3, number: 1, msg: "/__w/gdal/gdal/target/dem-hills-slope.tiff, band 1: IReadBlock failed at X offset 0, Y offset 0: TIFFReadEncodedStrip() failed." }

:exclamation:

lnicola commented 7 months ago

Ah, there's a race between tpi and slope. I can try to fix it tomorrow.

Should we use /vsimem/ here?

metasim commented 7 months ago

~@spadarian Will you accept this? https://github.com/spadarian/gdal/pull/12~

Thanks for merging! :)

lnicola commented 7 months ago

@metasim isn't that already merged?

metasim commented 7 months ago

@metasim isn't that already merged?

Oops! Yes. Sorry. Was looking at a non-refreshed page.

rouault commented 7 months ago

little robustness fix in https://github.com/spadarian/gdal/pull/13

spadarian commented 7 months ago

Rebased @lnicola