Migrate Variable integer type to 64-bit (#302)

* Migrate Variable integer type to 64-bit

Also fix #298 by checking for integer overflow when python ints are
passed, since they have arbitrary width.

Also bump pyupgrade version

* Consistently use std::runtime_error

Probably we should be using std::invalid_argument however

* Consistently use std::runtime_error

Probably should be std::invalid_argument

Also bump pre-commit as needed

* Move to ValueError consistently for mismatched inputs

Why not TypeError? Motivated by the fact that unpacking *args
into the wrong number is a ValueError.

* WINDOWS

Author: nsmith-

.pre-commit-config.yaml

@@ -1,6 +1,6 @@
 repos:
 - repo: https://github.com/psf/black
-  rev: 22.3.0
+  rev: 25.9.0
   hooks:
   - id: black
 
@@ -24,7 +24,7 @@ repos:
   - id: isort
 
 - repo: https://github.com/asottile/pyupgrade
-  rev: v2.31.0
+  rev: v3.21.0
   hooks:
   - id: pyupgrade
     args: ["--py39-plus"]

include/correction.h

@@ -17,7 +17,7 @@ class JSONObject; // internal wrapper around rapidjson
 class Variable {
   public:
     enum class VarType {string, integer, real};
-    typedef std::variant<int, double, std::string> Type;
+    typedef std::variant<int64_t, double, std::string> Type;
 
     Variable(const JSONObject& json);
     std::string name() const { return name_; };
@@ -225,7 +225,7 @@ class Category {
     double evaluate(const std::vector<Variable::Type>& values) const;
 
   private:
-    typedef std::map<int, Content> IntMap;
+    typedef std::map<int64_t, Content> IntMap;
     typedef std::map<std::string, Content> StrMap;
     std::variant<IntMap, StrMap> map_;
     std::unique_ptr<const Content> default_;

src/correction.cc

@@ -154,7 +154,7 @@ namespace {
   {
     double value = std::visit([](auto&& arg) -> double {
       using T = std::decay_t<decltype(arg)>;
-      if constexpr (std::is_same_v<T, int>) return static_cast<double>(arg);
+      if constexpr (std::is_same_v<T, int64_t>) return static_cast<double>(arg);
       else if constexpr (std::is_same_v<T, double>) return arg;
       else throw std::logic_error("I should not have ever seen a string");
       }, value_variant);
@@ -286,7 +286,7 @@ void Variable::validate(const Type& t) const {
       throw std::runtime_error("Input " + name() + " has wrong type: got string expected " + typeStr());
     }
   }
-  else if ( std::holds_alternative<int>(t) ) {
+  else if ( std::holds_alternative<int64_t>(t) ) {
     if ( type_ != VarType::integer ) {
       throw std::runtime_error("Input " + name() + " has wrong type: got int expected " + typeStr());
     }
@@ -398,8 +398,8 @@ double Transform::evaluate(const std::vector<Variable::Type>& values) const {
   if ( std::holds_alternative<double>(v) ) {
     v = vnew;
   }
-  else if ( std::holds_alternative<int>(v) ) {
-    v = (int) std::round(vnew);
+  else if ( std::holds_alternative<int64_t>(v) ) {
+    v = (int64_t) std::round(vnew);
   }
   else {
     throw std::logic_error("I should not have ever seen a string");
@@ -432,7 +432,7 @@ double HashPRNG::evaluate(const std::vector<Variable::Type>& values) const {
   size_t nbytes = sizeof(uint64_t)*variablesIdx_.size();
   uint64_t* seedData = (uint64_t*) alloca(nbytes);
   for(size_t i=0; i<variablesIdx_.size(); ++i) {
-    if ( auto v = std::get_if<int>(&values[variablesIdx_[i]]) ) {
+    if ( auto v = std::get_if<int64_t>(&values[variablesIdx_[i]]) ) {
       seedData[i] = static_cast<uint64_t>(*v);
     }
     else if ( auto v = std::get_if<double>(&values[variablesIdx_[i]]) ) {
@@ -652,7 +652,7 @@ double Category::evaluate(const std::vector<Variable::Type>& values) const {
       }
     }
   }
-  else if ( auto pval = std::get_if<int>(&values[variableIdx_]) ) {
+  else if ( auto pval = std::get_if<int64_t>(&values[variableIdx_]) ) {
     try {
       child = &std::get<IntMap>(map_).at(*pval);
     } catch (std::out_of_range& ex) {
@@ -696,11 +696,9 @@ double Correction::evaluate(const std::vector<Variable::Type>& values) const {
   if ( ! initialized_ ) {
     throw std::logic_error("Not initialized");
   }
-  if ( values.size() > inputs_.size() ) {
-    throw std::runtime_error("Too many inputs");
-  }
-  else if ( values.size() < inputs_.size() ) {
-    throw std::runtime_error("Insufficient inputs");
+  if ( values.size() != inputs_.size() ) {
+    throw std::invalid_argument("Incorrect number of inputs (got " + std::to_string(values.size())
+          + ", expected " + std::to_string(inputs_.size()) + ")");
   }
   for (size_t i=0; i < inputs_.size(); ++i) {
     inputs_[i].validate(values[i]);
@@ -767,11 +765,9 @@ size_t CompoundCorrection::input_index(const std::string_view name) const {
 }
 
 double CompoundCorrection::evaluate(const std::vector<Variable::Type>& values) const {
-  if ( values.size() > inputs_.size() ) {
-    throw std::runtime_error("Too many inputs");
-  }
-  else if ( values.size() < inputs_.size() ) {
-    throw std::runtime_error("Insufficient inputs");
+  if ( values.size() != inputs_.size() ) {
+    throw std::invalid_argument("Incorrect number of inputs (got " + std::to_string(values.size())
+          + ", expected " + std::to_string(inputs_.size()) + ")");
   }
   for (size_t i=0; i < inputs_.size(); ++i) {
     inputs_[i].validate(values[i]);

src/correctionlib/JSONEncoder.py

@@ -8,6 +8,7 @@
 Adapted from:
   https://stackoverflow.com/questions/16264515/json-dumps-custom-formatting
 """
+
 import gzip
 import json
 import math

src/correctionlib/_core/__init__.pyi

@@ -69,6 +69,7 @@ class FormulaAst:
     class BinaryOp:
         name: str
         value: int
+
     @property
     def nodetype(self) -> NodeType: ...
     @property

src/correctionlib/convert.py

@@ -1,8 +1,8 @@
-"""Tools to convert other formats to correctionlib
+"""Tools to convert other formats to correctionlib"""
 
-"""
+from collections.abc import Iterable, Sequence
 from numbers import Real
-from typing import TYPE_CHECKING, Any, Iterable, Optional, Sequence, Union, cast
+from typing import TYPE_CHECKING, Any, Optional, Union, cast
 
 import numpy
 
@@ -117,9 +117,11 @@ def build_data(
                     "content": [
                         {
                             "key": axes[0][i],
-                            "value": value
-                            if isinstance(value, Real)
-                            else build_data(value, axes[1:], variables[1:]),
+                            "value": (
+                                value
+                                if isinstance(value, Real)
+                                else build_data(value, axes[1:], variables[1:])
+                            ),
                         }
                         for i, value in enumerate(values)
                     ],
@@ -138,9 +140,11 @@ def build_data(
                     "edges": [edges(ax) for ax in axes[:i]],
                     "inputs": [var.name for var in variables[:i]],
                     "content": [
-                        value
-                        if isinstance(value, Real)
-                        else build_data(value, axes[i:], variables[i:])
+                        (
+                            value
+                            if isinstance(value, Real)
+                            else build_data(value, axes[i:], variables[i:])
+                        )
                         for value in flatten_to(values, i - 1)
                     ],
                     "flow": flow,
@@ -152,9 +156,11 @@ def build_data(
                 "input": variables[0].name,
                 "edges": edges(axes[0]),
                 "content": [
-                    value
-                    if isinstance(value, Real)
-                    else build_data(value, axes[1:], variables[1:])
+                    (
+                        value
+                        if isinstance(value, Real)
+                        else build_data(value, axes[1:], variables[1:])
+                    )
                     for value in values
                 ],
                 "flow": flow,

src/correctionlib/highlevel.py

@@ -1,9 +1,9 @@
-"""High-level correctionlib objects
+"""High-level correctionlib objects"""
 
-"""
 import json
+from collections.abc import Iterator, Mapping
 from numbers import Integral
-from typing import Any, Callable, Iterator, Mapping, Union
+from typing import Any, Callable, Union
 
 import numpy
 from packaging import version

src/python.cc

@@ -7,20 +7,40 @@ namespace py = pybind11;
 using namespace correction;
 
 namespace {
+  template<typename T> // Correction or CompoundCorrection
+  void check_length(const T& c, py::args args) {
+    // Error message should be the same as in Correction::evaluate
+    if ( py::len(args) != c.inputs().size() ) {
+      throw std::invalid_argument("Incorrect number of inputs (got " + std::to_string(py::len(args))
+          + ", expected " + std::to_string(c.inputs().size()) + ")");
+    }
+  }
+
+  template<typename T> // Correction or CompoundCorrection
+  std::vector<Variable::Type> validate_pyargs(const T& c, py::args args) {
+    // Ensure length before checking integer overflow
+    check_length(c, args);
+
+    // Check for integer overflow (py::cast converts to the double alternative)
+    for (size_t i=0; i < c.inputs().size(); ++i) {
+      if ( c.inputs()[i].type() == Variable::VarType::integer && py::isinstance<py::int_>(args[i]) ) {
+        py::cast<int64_t>(args[i]); // throws if out of range
+      }
+    }
+
+    return py::cast<std::vector<Variable::Type>>(args);
+  }
 
-  template<typename T>
+  template<typename T> // Correction or CompoundCorrection
   py::array_t<double> evalv(T& c, py::args args) {
     std::vector<Variable::Type> inputs;
     inputs.reserve(py::len(args));
     std::vector<std::pair<size_t, py::buffer_info>> vargs;
-    if ( py::len(args) != c.inputs().size() ) {
-      throw std::invalid_argument("Incorrect number of inputs (got " + std::to_string(py::len(args))
-          + ", expected " + std::to_string(c.inputs().size()) + ")");
-    }
+    check_length(c, args);
     for (size_t i=0; i < py::len(args); ++i) {
       if ( py::isinstance<py::array>(args[i]) ) {
         if ( c.inputs()[i].type() == Variable::VarType::integer ) {
-          vargs.emplace_back(i, py::cast<py::array_t<int, py::array::c_style | py::array::forcecast>>(args[i]).request());
+          vargs.emplace_back(i, py::cast<py::array_t<int64_t, py::array::c_style | py::array::forcecast>>(args[i]).request());
           inputs.emplace_back(0);
         }
         else if ( c.inputs()[i].type() == Variable::VarType::real ) {
@@ -52,8 +72,8 @@ namespace {
       py::gil_scoped_release release;
       for (long i=0; i < outbuffer.shape[0]; ++i) {
         for (const auto& varg : vargs) {
-          if ( std::holds_alternative<int>(inputs[varg.first]) ) {
-            inputs[varg.first] = static_cast<int*>(varg.second.ptr)[i];
+          if ( std::holds_alternative<int64_t>(inputs[varg.first]) ) {
+            inputs[varg.first] = static_cast<int64_t*>(varg.second.ptr)[i];
           }
           else if ( std::holds_alternative<double>(inputs[varg.first]) ) {
             inputs[varg.first] = static_cast<double*>(varg.second.ptr)[i];
@@ -65,7 +85,6 @@ namespace {
     return output;
   }
 }
-
 PYBIND11_MODULE(_core, m) {
     m.doc() = "python binding for corrections evaluator";
 
@@ -82,7 +101,7 @@ PYBIND11_MODULE(_core, m) {
         .def_property_readonly("inputs", &Correction::inputs)
         .def_property_readonly("output", &Correction::output)
         .def("evaluate", [](Correction& c, py::args args) {
-          return c.evaluate(py::cast<std::vector<Variable::Type>>(args));
+          return c.evaluate(validate_pyargs(c, args));
         })
         .def("evalv", evalv<Correction>);
 
@@ -92,7 +111,7 @@ PYBIND11_MODULE(_core, m) {
         .def_property_readonly("inputs", &CompoundCorrection::inputs)
         .def_property_readonly("output", &CompoundCorrection::output)
         .def("evaluate", [](CompoundCorrection& c, py::args args) {
-          return c.evaluate(py::cast<std::vector<Variable::Type>>(args));
+          return c.evaluate(validate_pyargs(c, args));
         })
         .def("evalv", evalv<CompoundCorrection>);
 

tests/test_core.py

@@ -49,7 +49,7 @@ def test_evaluator():
     assert sf.version == 2
     assert sf.description == ""
 
-    with pytest.raises(RuntimeError):
+    with pytest.raises(ValueError):
         sf.evaluate(0, 1.2, 35.0, 0.01)
 
     assert sf.evaluate() == 1.234
@@ -110,21 +110,21 @@ def test_evaluator():
     assert sf.version == 2
     assert sf.description == ""
 
-    with pytest.raises(RuntimeError):
+    with pytest.raises(ValueError):
         # too many inputs
         sf.evaluate(0, 1.2, 35.0, 0.01)
 
-    with pytest.raises(RuntimeError):
+    with pytest.raises(ValueError):
         # not enough inputs
         sf.evaluate(1.2)
 
     with pytest.raises(RuntimeError):
         # wrong type
-        sf.evaluate(5)
+        sf.evaluate(5, "blah")
 
     with pytest.raises(RuntimeError):
         # wrong type
-        sf.evaluate("asdf")
+        sf.evaluate("asdf", "blah")
 
     assert sf.evaluate(12.0, "blah") == 1.1
     # Do we need pytest.approx? Maybe not

tests/test_highlevel.py

@@ -35,7 +35,7 @@ def test_highlevel(cset):
     assert sf.version == 2
     assert sf.description == ""
 
-    with pytest.raises(RuntimeError):
+    with pytest.raises(ValueError):
         sf.evaluate(0, 1.2, 35.0, 0.01)
 
     assert sf.evaluate(1.0, 1.0) == 1.234