bug: histogram(nbins=N) off by one error

[lostmygithubaccount]

What happened?

import ibis

ibis.options.interactive = True
ibis.options.repr.interactive.max_rows = 20

t = ibis.range(1000).unnest().name("index").as_table()
t.select(hist=t["index"].histogram(nbins=10)).value_counts()

┏━━━━━━━┳━━━━━━━━━━━━┓
┃ hist  ┃ hist_count ┃
┡━━━━━━━╇━━━━━━━━━━━━┩
│ int64 │ int64      │
├───────┼────────────┤
│     5 │        100 │
│     9 │         99 │
│     2 │        100 │
│    10 │          1 │
│     0 │        100 │
│     1 │        100 │
│     3 │        100 │
│     6 │        100 │
│     4 │        100 │
│     7 │        100 │
│     8 │        100 │
└───────┴────────────┘

there is one value in bin 10 and 99 in bin 9

What version of ibis are you using?

main

What backend(s) are you using, if any?

duckdb

Relevant log output

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[cpcloud]

Yeah, looks like a bug! Great example, really makes the problem obvious :)

[christophertitchen]

/take

[jitingxu1]

Took a quick look at the code, it'll always put the maximum value of the column in an extra bin based on the current design (self - base) / binwidth.

I'm not sure about the eps, seems like not having real effect on the bin assignment. It slightly alters the binwidth and the value, effectively canceling out any impact.

Also, the base is the min value of the bins, I feel like make more sense to have range(min, max) if we want to let user specify the min and max value of bins.

[christophertitchen]

Took a quick look at the code, it'll always put the maximum value of the column in an extra bin based on the current design (self - base) / binwidth.

I'm not sure about the eps, seems like not having real effect on the bin assignment. It slightly alters the binwidth and the value, effectively canceling out any impact.

Also, the base is the min value of the bins, I feel like make more sense to have range(min, max) if we want to let user specify the min and max value of bins.

Yes, I agree it would be lovely to have that functionality to specify the lower and upper range of the bins, along with a few other useful features like non-uniform bin-widths and weights like in np.histogram. This will be a quick and straightforward fix within the current design though—to make the bounds of the last bin inclusive—so I thought it would make a nice first contribution for me. We can save the more comprehensive redesign for another PR once the bug has been fixed.