Add histogram calculation for RasterBand #468

spadarian · 2023-11-19T23:36:50Z

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 exposes 2 GDAL methods to calculate band histograms (default and user-defined).

src/raster/rasterband.rs

metasim · 2023-11-26T23:44:17Z

src/raster/rasterband.rs

+        self.max
+    }
+    /// Histogram values for each bucket
+    pub fn histogram(&self) -> &[i32] {


Consider naming buckets or counts? With this name, you'll end up with things like this:

let rb: Rasterband<'_> = ...; let histogram = rb.get_histogram(...); for count in histogram.histogram() { // <--- awkward. ... }

I also found it weird but I was just being consistent with GDAL naming. Happy to change it if we are not aiming for that.

metasim · 2023-11-26T23:48:07Z

src/raster/rasterband.rs

+pub struct Histogram {
+    min: f64,
+    max: f64,
+    n_buckets: i32,


Why isn't this usize? Sometimes GDAL uses signed values when it doesn't make semantic sense, and there's precedence for rectifying this in Rust.

I haven't contributed for a while and from memory we were using isize. I do agree that we could rectify that.

How far do we want to take it? I could also use a NonZeroUsize, which could be also useful when selecting a band from a dataset... (@lnicola)

Lol... yeah, I've argued for NonZeroUsize, but creating them is really annoying/verbose.

When I see isize or i32 I immediately ask myself "What does a negative value mean?". If it has meaning, we should document it. If it doesn't, then I'd at least try in the bindings to coerce it into an unsigned type.

Yeah, I wish constructing a NonZeroUsize from a constant/literal was possible without unsafe or bloating the binary. 😔

metasim · 2023-11-26T23:49:34Z

src/raster/rasterband.rs

+    histogram: *mut i32,
+    histogram_vec: Option<Vec<i32>>,


Is this storing the counts twice? Who owns histogram?

It would really help to document why this is neccessary. :)

Yes, sorry. I should document that. I used that double storage mechanisms because with GDALGetDefaultHistogram C owns histogram and with GDALGetRasterHistogram Rust does.

Here's a restating of my idea from before. Does this capture the semantics? I should probably just compile and step through the tests, but I'm admittedly confused if I've gotten the borrowed vs owned cases switched...

pub struct Histogram { min: f64, max: f64, n_buckets: i32, counts: HistCounts } enum HistCounts { Borrowed(*mut libc::c_void), Owned(Vec<i32>) } ... impl Drop for HistCounts { fn drop(&mut self) { match self { Self::Borrowed(p) => ???; Self::Owned(v) => ???; } } }

No worries mate. I'm pushing those changes now.

src/raster/rasterband.rs

metasim · 2023-11-26T23:53:04Z

src/raster/rasterband.rs

+    /// # Arguments
+    ///
+    /// * `force` - If `true`, force the computation. If `false` and no default histogram is available, the method will return None
+    pub fn get_default_histogram(&self, force: bool) -> Result<Option<Histogram>> {


For coherence with other methods/functions, I'd consider collapsing these two methods into a single one accepting a new HistogramOptions with a Default implementation setting the GDALGetDefaultHistogram settings.

@lnicola Do you agree with me? Even though there are separate GDALGetDefaultHistogram and GDALGetRasterHistogram binding functions, we could be more idiomatic with the HistogramOptions approach.

I do prefer thin wrappers to avoid learning a new API but I'm happy to make it more idiomatic if you guys want.

Fair point. Let's hear what some of the other contributors think. I'm on the extreme of "let's not leak C-language workarounds" and "let's leverage Rust to improve ergonomics". Others, I think, are more in your camp, so let's let someone else make the call. We've had related debates here.

src/raster/rasterband.rs

metasim · 2023-11-27T00:02:18Z

src/raster/rasterband.rs

+    max: f64,
+    n_buckets: i32,
+    histogram: *mut i32,
+    histogram_vec: Option<Vec<i32>>,


I personally think the counts should be usize or u32, regardless of what GDAL C returns, but not sure that's a consensus opinion. @lnicola @jdroenner your opinion?

I saw that @lnicola did something similar with raster index, so probably it makes sense to do it like that moving forward.

CHANGES.md

spadarian · 2023-11-27T23:43:40Z

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

Double storage mechanism for Borrowed and Owned histogram counts (makes sense and it is hidden from the user)
Changing isize to usize since we recently changed that for raster indexing

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

src/raster/rasterband.rs

metasim · 2023-11-29T17:38:22Z

@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.

src/raster/rasterband.rs

CHANGES.md

src/raster/rasterband.rs

lnicola · 2023-11-29T18:25:29Z

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

spadarian · 2023-11-29T20:13:30Z

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

lnicola · 2023-11-29T20:15:16Z

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." }

❗

lnicola · 2023-11-29T20:23:45Z

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

Should we use /vsimem/ here?

metasim · 2023-11-30T19:42:08Z

~~@spadarian Will you accept this? spadarian#12~~

Thanks for merging! :)

lnicola · 2023-11-30T19:42:49Z

@metasim isn't that already merged?

metasim · 2023-11-30T19:46:34Z

@metasim isn't that already merged?

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

rouault · 2023-11-30T22:18:36Z

little robustness fix in spadarian#13

…n GDAL

spadarian · 2023-12-01T01:22:19Z

Rebased @lnicola

src/raster/rasterband.rs

lnicola · 2023-12-01T14:00:12Z

src/raster/rasterband.rs

+    /// Pointer to GDAL allocated array and lenght of histogram counts.
+    ///
+    /// Requires freeing with [`VSIFree`][gdal_sys::VSIFree].
+    GdalAllocated(*mut u64, usize),


@metasim this seems to use u64 consistently for the counts, did you have some concerns about that?

No, sounds fine. I just had comments about it to make sure we used the .*Ex variants in boths cases.

src/raster/rasterband.rs

spadarian force-pushed the histogram branch from 55ef313 to c87c1cf Compare November 20, 2023 01:55

metasim reviewed Nov 26, 2023

View reviewed changes

src/raster/rasterband.rs Outdated Show resolved Hide resolved

metasim reviewed Nov 26, 2023

View reviewed changes

src/raster/rasterband.rs Outdated Show resolved Hide resolved

metasim reviewed Nov 26, 2023

View reviewed changes

src/raster/rasterband.rs Show resolved Hide resolved

metasim reviewed Nov 27, 2023

View reviewed changes

src/raster/rasterband.rs Outdated Show resolved Hide resolved

metasim reviewed Nov 27, 2023

View reviewed changes

CHANGES.md Show resolved Hide resolved

metasim reviewed Nov 28, 2023

View reviewed changes

src/raster/rasterband.rs Outdated Show resolved Hide resolved

metasim reviewed Nov 29, 2023

View reviewed changes

src/raster/rasterband.rs Show resolved Hide resolved

metasim reviewed Nov 29, 2023

View reviewed changes

src/raster/rasterband.rs Outdated Show resolved Hide resolved

lnicola reviewed Nov 29, 2023

View reviewed changes

src/raster/rasterband.rs Outdated Show resolved Hide resolved

lnicola reviewed Nov 29, 2023

View reviewed changes

src/raster/rasterband.rs Outdated Show resolved Hide resolved

lnicola reviewed Nov 29, 2023

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

lnicola reviewed Nov 29, 2023

View reviewed changes

src/raster/rasterband.rs Show resolved Hide resolved

lnicola reviewed Nov 29, 2023

View reviewed changes

src/raster/rasterband.rs Outdated Show resolved Hide resolved

lnicola reviewed Nov 29, 2023

View reviewed changes

src/raster/rasterband.rs Outdated Show resolved Hide resolved

lnicola mentioned this pull request Nov 30, 2023

Fix tpi test output name and use /vsimem/ for DEM tests #469

Merged

2 tasks

spadarian and others added 5 commits December 1, 2023 12:01

Add histogram calculation for RasterBand

5ed24bf

Refactor Histogram counts

b546b93

Alternative naming of HistCounts variants. Additional documentation.

6e152dd

Change to new C histogram API

a7491be

RasterBand::histogram(): error out if n_buckets == 0 to avoid crash i…

246cf4e

…n GDAL

spadarian force-pushed the histogram branch from 674ffe8 to 246cf4e Compare December 1, 2023 01:03

Fix format error

1392a9d

lnicola reviewed Dec 1, 2023

View reviewed changes

src/raster/rasterband.rs Outdated Show resolved Hide resolved

lnicola reviewed Dec 1, 2023

View reviewed changes

src/raster/rasterband.rs Outdated Show resolved Hide resolved

Add as_slice method for HistogramCounts

d2960a2

lnicola approved these changes Dec 2, 2023

View reviewed changes

lnicola merged commit d279c9f into georust:master Dec 2, 2023
9 checks passed

spadarian deleted the histogram branch December 2, 2023 18:17

spadarian mentioned this pull request Dec 3, 2023

Add method to set default histogram for RasterBand #486

Merged

2 tasks

lnicola mentioned this pull request Apr 29, 2024

Polygonize wrapper #533

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add histogram calculation for RasterBand #468

Add histogram calculation for RasterBand #468

spadarian commented Nov 19, 2023

metasim Nov 26, 2023 •

edited

Loading

spadarian Nov 27, 2023

metasim Nov 26, 2023

spadarian Nov 27, 2023

metasim Nov 27, 2023

lnicola Nov 29, 2023

metasim Nov 26, 2023 •

edited

Loading

spadarian Nov 27, 2023

metasim Nov 27, 2023

spadarian Nov 27, 2023

metasim Nov 26, 2023 •

edited

Loading

spadarian Nov 27, 2023

metasim Nov 27, 2023

metasim Nov 27, 2023 •

edited

Loading

spadarian Nov 27, 2023

spadarian commented Nov 27, 2023

metasim commented Nov 29, 2023

lnicola commented Nov 29, 2023

spadarian commented Nov 29, 2023

lnicola commented Nov 29, 2023

lnicola commented Nov 29, 2023 •

edited

Loading

metasim commented Nov 30, 2023 •

edited

Loading

lnicola commented Nov 30, 2023

metasim commented Nov 30, 2023

rouault commented Nov 30, 2023

spadarian commented Dec 1, 2023

lnicola Dec 1, 2023

metasim Dec 1, 2023

Add histogram calculation for RasterBand #468

Add histogram calculation for RasterBand #468

Conversation

spadarian commented Nov 19, 2023

metasim Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metasim Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metasim Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metasim Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadarian commented Nov 27, 2023

metasim commented Nov 29, 2023

lnicola commented Nov 29, 2023

spadarian commented Nov 29, 2023

lnicola commented Nov 29, 2023

lnicola commented Nov 29, 2023 • edited Loading

metasim commented Nov 30, 2023 • edited Loading

lnicola commented Nov 30, 2023

metasim commented Nov 30, 2023

rouault commented Nov 30, 2023

spadarian commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metasim Nov 26, 2023 •

edited

Loading

metasim Nov 26, 2023 •

edited

Loading

metasim Nov 26, 2023 •

edited

Loading

metasim Nov 27, 2023 •

edited

Loading

lnicola commented Nov 29, 2023 •

edited

Loading

metasim commented Nov 30, 2023 •

edited

Loading