Isis3: hist application does not use the user specified nbin parameter correctly.

[lavalaz]

ISIS version(s) affected: x.y.z
isis4.1.0 (and earlier)
Description

I carefully picked the min/max values and NBINS to put the bin boundaries at very specific DN values. But the application did not honor those user input limits and chose a min value larger than the user specified min and a max value less than the user specified max. Therefore the bin boundaries are not where the user intended.
How to reproduce

Pick any typical image with I/F values between 0 and 1.
_hist from=[input.cub] minimum=0.0 maximum=1.0 nbin=10_
This should but the bin boundaries at 0.0-0.1, 0.1-0.2, 0.2-0.3, 0.3-0.4, 0.4-0.5, 0.5-0.6, 0.6-0.7, 0.7-0.8, 0.8-0.9, 0.9-1.0. Count them. Ten (10) bins. Hist produced 10 bins with "DN" of 0,0.11111,0.222222,0.333333,0.444444... ...0.88888,1. Ten bins alright, but not at 0.1 spacing. I guess I was wrong and 1/10=0.111111. My bad. So what DN's are in those bins? The output only specifies one DN value and I can't tell if its the max or the min of the bin. Since it starts with "0", clearly its the minimum value and the first bin has DN values of 0.0-0.111111. But since the last bin is "1", it must have DN values of 1 to ??? Oh, must be my mistake, the DN value must be the for the top of the bin so the last bin is for DN values of 0.88888-1.0. So the first bin has DNs of ??? to zero? But if I ask for 11 bins, I get bins of "0,0.1,0.2,0.3...1". 1/11=0.1, now I get it!
Possible Solution

Part 1: You need to give the values for minimum and maximum of the range of DNs that are going into each bin, not just a single DN value. You should also clearly document which side of the bin is a greater than/less than versus greater-than-or-equal-to/less-than-or-equal-to.
Part 2: You should also have a bin for "less-than-the-minimum" and "greater-than-the-maximum". So 12, clearly labeled, bins when the user says nbin=10.
Additional context

Just for historical context, I know this issue was raised about 15 years ago. The user in question, not myself, who had a degree in mathematics, was told that they did not understand the mathematics of histograms and this behavior was correct. That is why you are seeing extra snarkiness in this ticket.

[jessemapel]

Just for historical context, I know this issue was raised about 15 years ago. The user in question, not myself, who had a degree in mathematics, was told that they did not understand the mathematics of histograms and this behavior was correct. That is why you are seeing extra snarkiness in this ticket.

The snarkiness is understandable, but it makes your bug report harder to read. The person your talking about doesn't work on ISIS anymore, so you're directing your snark towards people who are not related to it at all. I'd appreciate it if you removed the snarkiness from your bug report next time so that it's less likely to be misunderstood.

[lavalaz]

Yes, that is a fair request. I will try to do my best to not bring up things that happened a generation ago!

[jessemapel]

I agree this math looks incorrect to me:

index = (int) floor((double)(nbins - 1) / (BinRangeEnd() - BinRangeStart() ) *
                            (data - BinRangeStart() ) + 0.5);

[kaitlyndlee]

@lavalaz Would it be possible for you to share a cube that would be good for testing, possibly the one you used in the original post?

[lavalaz]

Tied up in meetings, will try to get you one this evening. Will give you a link to one on /work somewhere...

[kaitlyndlee]

@lavalaz You mentioned adding two extra bins to "catch" values outside of the range specified by the user. Would adding a parameter that allows users to select if they want the two extra bins be useful? Is there a use case for this functionality? It looks like the current functionality places values outside of the range in the first and last bins, e.g., bins 1 and 10 in the example.

In addition, would changing the CSV output to:

MinInclusive,MaxExclusive,Pixels,CumulativePixels,Percent,CumulativePercent

from:

DN,Pixels,CumulativePixels,Percent,CumulativePercent

meet the requirements of showing the bounds of each bin? Could this change negatively impact users? I am not familiar with hist or if it is used in any pipelines.

Lastly, some of these changes will affect other programs, e.g., cnethist, since they all use the same class in the background: Histogram. I assume we would need to update the output for all of these programs, as well. It looks like cnethist is the only other one that is used to generate histograms, is this correct?

@jessemapel If you have any opinions about this too.

[lavalaz]

You could make it a switch or just have them there always. There may be some users who complain about "extra" bins if they asked for a specific set of bins, so its not a bad idea but probably not essential.

[lavalaz]

After discussions and understanding more of how _hist_ works, I am clarifying desired output:
Histogram with the number of bins the users asked for (no extra bins), no pixels below the requested _min_ or above the _max_ are included in the histogram.
Number of valid pixels below the _min_ and number of valid pixels above _max_ reported in the output. Total number of pixels and total valid pixels should both also be reported in the output, at it already is.

[kaitlyndlee]

To clarify, I was wrong about pixels outside of the range being placed in the first and last bins. I missed a line of code and did some testing. However, we will still need to add how many pixels are outside of the range.

[jessemapel]

Because of how this change impacts the output format for hist, we're pushing the release of this bug fix back to August with the full feature release in 4.2

@lavalaz If you need this fix before then, let us know and we'll stage an internal build for you to use.

[blandoplanet]

The release candidate for 4.2 will be available in early July?

[jessemapel]

@blandoplanet Yes

[lavalaz]

I don't need a special early version. Thank you!

[kberryUSGS]

PR needed to be temporarily reverted due to lots of failing tests. See PR #3922.

[jessemapel]

Fixed by #4004