fix: (better) error when filling an int with a float (#876)

* fix: fix int fill with float rounding issue

Signed-off-by: Henry Schreiner <[email protected]>

* fix: cast to array first

Signed-off-by: Henry Schreiner <[email protected]>

* fix: disallow float filling for integer axes

Signed-off-by: Henry Schreiner <[email protected]>

---------

Signed-off-by: Henry Schreiner <[email protected]>

Author: henryiii

docs/user-guide/storage.rst

@@ -84,7 +84,7 @@ Mean
 
 This storage tracks a "Profile", that is, the mean value of the accumulation instead of the sum.
 It stores the count (as a double), the mean, and a term that is used to compute the variance. When
-filling, you can add a ``sample=`` term.
+filling, you must add a ``sample=`` term.
 
 
 WeightedMean

include/bh_python/fill.hpp

@@ -95,6 +95,29 @@ inline decltype(auto) special_cast<c_array_t<std::string>>(py::handle x) {
     return py::cast<B>(x);
 }
 
+// Make sure float arrays don't get cast to integers (-.5 rounds to 0!)
+template <>
+inline decltype(auto) special_cast<c_array_t<int>>(py::handle x) {
+    auto np    = py::module::import("numpy");
+    auto dtype = py::cast<py::array>(x).dtype();
+    if(dtype.equal(np.attr("bool_")) || dtype.equal(np.attr("int8"))
+       || dtype.equal(np.attr("int16")) || dtype.equal(np.attr("int32"))
+       || dtype.equal(np.attr("int64")))
+        return py::cast<c_array_t<int>>(x);
+    throw py::type_error("Only integer arrays supported when targeting integer axes");
+}
+
+// Produce a type error for passing float to int
+template <>
+inline decltype(auto) special_cast<int>(py::handle x) {
+    try {
+        return py::cast<int>(x);
+    } catch(std::runtime_error&) {
+        throw py::type_error(
+            "Only integer values supported when targeting integer axes");
+    }
+}
+
 using arg_t = variant::variant<c_array_t<double>,
                                double,
                                c_array_t<int>,

tests/test_histogram.py

@@ -117,6 +117,18 @@ def test_fill_int_1d():
         h[-3]
 
 
+def test_fill_int_with_float_single_1d():
+    h = bh.Histogram(bh.axis.Integer(-1, 2))
+    with pytest.raises(TypeError):
+        h.fill(0.3)
+
+
+def test_fill_int_with_float_array_1d():
+    h = bh.Histogram(bh.axis.Integer(-1, 2))
+    with pytest.raises(TypeError):
+        h.fill([-0.3, 0.3])
+
+
 def test_fill_1d(flow):
     h = bh.Histogram(bh.axis.Regular(3, -1, 2, underflow=flow, overflow=flow))
     with pytest.raises(ValueError):
@@ -935,7 +947,7 @@ def ia(*args):
         a.fill(np.empty(2), 1)
     with pytest.raises(ValueError):
         a.fill(np.empty(2), np.empty(3))
-    with pytest.raises(ValueError):
+    with pytest.raises(TypeError):
         a.fill("abc")
 
     with pytest.raises(IndexError):
@@ -976,12 +988,9 @@ def ia(*args):
     platform.machine() == "ppc64le", reason="ppc64le bug (TBD)", strict=False
 )
 def test_fill_with_sequence_1():
-    def fa(*args):
-        return np.array(args, dtype=float)
-
     a = bh.Histogram(bh.axis.Integer(0, 3), storage=bh.storage.Weight())
-    v = fa(-1, 0, 1, 2, 3, 4)
-    w = fa(2, 3, 4, 5, 6, 7)
+    v = np.array([-1, 0, 1, 2, 3, 4], dtype=int)
+    w = np.array([2, 3, 4, 5, 6, 7], dtype=float)
     a.fill(v, weight=w)
     a.fill((0, 1), weight=(2, 3))
 

tests/test_internal_histogram.py

@@ -280,7 +280,7 @@ def test_int_cat_hist():
     assert_array_equal(h.view(), [1, 1, 1])
     assert h.sum() == 3
 
-    with pytest.raises(RuntimeError):
+    with pytest.raises(TypeError):
         h.fill(0.5)