Repository: xgcm/xhistogram

Issue:
Align definition of `bins` with numpy.histogram
When bin edges are specified in `numpy.histogram`, the last bin is closed on both sides. From the `numpy` docs (https://numpy.org/doc/stable/reference/generated/numpy.histogram.html):
> All but the last (righthand-most) bin is half-open. In other words, if bins is:
> 
> [1, 2, 3, 4]
> then the first bin is [1, 2) (including 1, but excluding 2) and the second [2, 3). The last bin, however, is [3, 4], which includes 4.

This is not the case for `xhistogram`, e.g.:
```python
import numpy as np
import xarray as xr
from xhistogram.xarray import histogram as xhist

data = np.ones(10)
da =  xr.DataArray(data, coords=[range(10)], dims=['x']).rename('test')

bins = np.array([0, 0.5, 1])

print(f'xhistogram: {xhist(da, bins=[bins]).values}')
print(f'numpy histogram: {np.histogram(data, bins=bins)[0]}')
```
```
xhistogram: [0 0]
numpy histogram: [ 0 10]
```

Could we make it the case?

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/xgcm/xhistogram/pull/20?src=pr&el=h1) Report
> Merging [#20](https://codecov.io/gh/xgcm/xhistogram/pull/20?src=pr&el=desc) into [master](https://codecov.io/gh/xgcm/xhistogram/commit/4ae9f0a5afd50bf2c143828e6114bfcbbcce905b?el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/xgcm/xhistogram/pull/20/graphs/tree.svg?width=650&height=150&src=pr&token=b3kGT5oBSH)](https://codecov.io/gh/xgcm/xhistogram/pull/20?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
+ Coverage   94.22%   94.24%   +0.02%     
==========================================
  Files           3        3              
  Lines         225      226       +1     
  Branches       64       65       +1     
==========================================
+ Hits          212      213       +1     
  Misses          8        8              
  Partials        5        5              
```


| [Impacted Files](https://codecov.io/gh/xgcm/xhistogram/pull/20?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [xhistogram/xarray.py](https://codecov.io/gh/xgcm/xhistogram/pull/20/diff?src=pr&el=tree#diff-eGhpc3RvZ3JhbS94YXJyYXkucHk=) | `91.93% <ø> (ø)` | |
| [xhistogram/core.py](https://codecov.io/gh/xgcm/xhistogram/pull/20/diff?src=pr&el=tree#diff-eGhpc3RvZ3JhbS9jb3JlLnB5) | `98.49% <100.00%> (+0.01%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/xgcm/xhistogram/pull/20?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/xgcm/xhistogram/pull/20?src=pr&el=footer). Last update [4ae9f0a...790e241](https://codecov.io/gh/xgcm/xhistogram/pull/20?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/doc/contributing.rst b/doc/contributing.rst
index cc351d3..80841b5 100644
--- a/doc/contributing.rst
+++ b/doc/contributing.rst
@@ -8,6 +8,12 @@ GitHub repo: `https://github.com/xgcm/xhistogram <https://github.com/xgcm/xhisto
 Release History
 ---------------
 
+v0.1.?
+~~~~~
+
+- Aligned definition of `bins` with `numpy.histogram` (:pr:`???`)
+  By `Dougie Squire <https://github.com/dougiesquire>`_.
+
 v0.1.1
 ~~~~~~
 
diff --git a/xhistogram/core.py b/xhistogram/core.py
index 9f992c2..559d1a2 100644
--- a/xhistogram/core.py
+++ b/xhistogram/core.py
@@ -107,6 +107,15 @@ def _histogram_2d_vectorized(*args, bins=None, weights=None, density=False,
     # https://github.com/numpy/numpy/blob/9c98662ee2f7daca3f9fae9d5144a9a8d3cabe8c/numpy/lib/histograms.py#L864-L882
     # for now we stick with `digitize` because it's easy to understand how it works
 
+    # Add small increment to the last bin edge to make the final bin right-edge inclusive
+    # Note, this is the approach taken by sklearn, e.g.
+    # https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/calibration.py#L592
+    # but a better approach would be to use something like _search_sorted_inclusive() in 
+    # numpy histogram. This is an additional motivation for moving to searchsorted
+    bins = [np.concatenate((
+        b[:-1],
+        b[-1:] + 1e-8)) for b in bins]
+    
     # the maximum possible value of of digitize is nbins
     # for right=False:
     #   - 0 corresponds to a < b[0]
@@ -154,6 +163,9 @@ def histogram(*args, bins=None, axis=None, weights=None, density=False,
           * A combination [int, array] or [array, int], where int
             is the number of bins and array is the bin edges.
 
+        When bin edges are specified, all but the last (righthand-most) bin include
+        the left edge and exclude the right edge. The last bin includes both edges.
+        
         A ``TypeError`` will be raised if ``args`` contains dask arrays and
         ``bins`` are not specified explicitly as a list of arrays.
     axis : None or int or tuple of ints, optional
diff --git a/xhistogram/xarray.py b/xhistogram/xarray.py
index cd9b65f..2dc8ba9 100644
--- a/xhistogram/xarray.py
+++ b/xhistogram/xarray.py
@@ -31,6 +31,9 @@ def histogram(*args, bins=None, dim=None, weights=None, density=False,
           * A combination [int, array] or [array, int], where int
             is the number of bins and array is the bin edges.
 
+        When bin edges are specified, all but the last (righthand-most) bin include
+        the left edge and exclude the right edge. The last bin includes both edges.
+        
         A ``TypeError`` will be raised if ``args`` contains dask arrays and
         ``bins`` are not specified explicitly as a list of arrays.
     dim : tuple of strings, optional


Test patch:
diff --git a/xhistogram/test/test_core.py b/xhistogram/test/test_core.py
index a5b0a05..e4802fb 100644
--- a/xhistogram/test/test_core.py
+++ b/xhistogram/test/test_core.py
@@ -79,6 +79,27 @@ def test_histogram_results_1d_weighted_broadcasting(block_size):
     np.testing.assert_array_equal(2*h, h_w)
 
 
+@pytest.mark.parametrize('block_size', [None, 1, 2])
+def test_histogram_right_edge(block_size):
+    """Test that last bin is both left- and right-edge inclusive as it 
+        is for numpy.histogram
+    """
+    nrows, ncols = 5, 20
+    data = np.ones((nrows, ncols))
+    bins = np.array([0, 0.5, 1]) # All data at rightmost edge
+
+    h = histogram(data, bins=bins, axis=1, block_size=block_size)
+    assert h.shape == (nrows, len(bins)-1)
+
+    # make sure we get the same thing as histogram (all data in the last bin)
+    hist, _ = np.histogram(data, bins=bins)
+    np.testing.assert_array_equal(hist, h.sum(axis=0))
+
+    # now try with no axis
+    h_na = histogram(data, bins=bins, block_size=block_size)
+    np.testing.assert_array_equal(hist, h_na)
+    
+
 def test_histogram_results_2d():
     nrows, ncols = 5, 20
     data_a = np.random.randn(nrows, ncols)