accumulators: reject delta_N < 1 in core ctor and SI setter (bug-sweep #24)

src/core/accumulators/AccumulatorBase.hpp

@@ -21,6 +21,7 @@
 
 #include <cassert>
 #include <cstddef>
+#include <stdexcept>
 #include <string>
 #include <vector>
 
@@ -37,7 +38,11 @@ namespace Accumulators {
 class AccumulatorBase {
 public:
   AccumulatorBase(::System::System const *system, int delta_N)
-      : m_system(reinterpret_cast<void const *>(system)), m_delta_N(delta_N) {}
+      : m_system(reinterpret_cast<void const *>(system)), m_delta_N(delta_N) {
+    if (delta_N < 1) {
+      throw std::runtime_error("delta_N must be >= 1");
+    }
+  }
   virtual ~AccumulatorBase() = default;
 
   int &delta_N() { return m_delta_N; }

src/script_interface/accumulators/AccumulatorBase.hpp

@@ -27,6 +27,7 @@
 #include "core/system/System.hpp"
 
 #include <memory>
+#include <stdexcept>
 #include <string>
 #include <vector>
 
@@ -38,7 +39,13 @@ class AccumulatorBase : public AutoParameters<AccumulatorBase> {
   AccumulatorBase() {
     add_parameters({{"delta_N",
                      [this](const Variant &v) {
-                       accumulator()->delta_N() = get_value<int>(v);
+                       auto const delta_N = get_value<int>(v);
+                       context()->parallel_try_catch([&]() {
+                         if (delta_N < 1) {
+                           throw std::runtime_error("delta_N must be >= 1");
+                         }
+                       });
+                       accumulator()->delta_N() = delta_N;
                      },
                      [this]() { return accumulator()->delta_N(); }}});
   }

src/script_interface/accumulators/MeanVarianceCalculator.hpp

@@ -44,9 +44,12 @@ class MeanVarianceCalculator : public AccumulatorBase {
     set_from_args(m_obs, params, "obs");
 
     if (m_obs)
-      m_accumulator = std::make_shared<::Accumulators::MeanVarianceCalculator>(
-          get_core_system_pointer(params),
-          get_value_or<int>(params, "delta_N", 1), m_obs->observable());
+      ObjectHandle::context()->parallel_try_catch([&]() {
+        m_accumulator =
+            std::make_shared<::Accumulators::MeanVarianceCalculator>(
+                get_core_system_pointer(params),
+                get_value_or<int>(params, "delta_N", 1), m_obs->observable());
+      });
   }
 
   std::shared_ptr<::Accumulators::MeanVarianceCalculator>

src/script_interface/accumulators/TimeSeries.hpp

@@ -42,9 +42,11 @@ class TimeSeries : public AccumulatorBase {
     set_from_args(m_obs, params, "obs");
 
     if (m_obs)
-      m_accumulator = std::make_shared<::Accumulators::TimeSeries>(
-          get_core_system_pointer(params),
-          get_value_or<int>(params, "delta_N", 1), m_obs->observable());
+      ObjectHandle::context()->parallel_try_catch([&]() {
+        m_accumulator = std::make_shared<::Accumulators::TimeSeries>(
+            get_core_system_pointer(params),
+            get_value_or<int>(params, "delta_N", 1), m_obs->observable());
+      });
   }
 
   Variant do_call_method(std::string const &method,

testsuite/python/accumulator_correlator.py

@@ -315,6 +315,20 @@ def create_accumulator(obs1=obs, **kwargs):
             create_accumulator(tau_lin=3)
         with self.assertRaisesRegex(RuntimeError, "tau_max must be >= delta_t"):
             create_accumulator(delta_N=2 * int(10. / self.system.time_step))
+        # delta_N <= 0 is meaningless and must be rejected at construction
+        # (otherwise delta_t=0 leads to log2(inf) UB in the Correlator)
+        with self.assertRaisesRegex(RuntimeError, "delta_N must be >= 1"):
+            create_accumulator(delta_N=0)
+        with self.assertRaisesRegex(RuntimeError, "delta_N must be >= 1"):
+            create_accumulator(delta_N=-1)
+        # the runtime setter must reject invalid values too
+        acc = create_accumulator()
+        with self.assertRaisesRegex(RuntimeError, "delta_N must be >= 1"):
+            acc.delta_N = 0
+        with self.assertRaisesRegex(RuntimeError, "delta_N must be >= 1"):
+            acc.delta_N = -3
+        # a rejected setter call must leave the previous value intact
+        self.assertEqual(acc.delta_N, 1)
         with self.assertRaisesRegex(ValueError, "correlation operation 'unknown' not implemented"):
             create_accumulator(corr_operation="unknown")
         with self.assertRaisesRegex(ValueError, "unknown compression method 'unknown1' for first observable"):
@@ -365,6 +379,25 @@ def create_accumulator(obs1=obs, **kwargs):
         with self.assertRaisesRegex(RuntimeError, "auto_update_accumulators property"):
             pickle.dumps(acc)
 
+    def test_delta_N_validation(self):
+        """delta_N <= 0 must be rejected by all accumulators (construction and
+        runtime setter). With delta_N=0 an auto-update would hang the
+        integrator (frequency=0) and the Correlator hits UB at construction."""
+        self.system.part.add(pos=[(0, 0, 0)])
+        obs = espressomd.observables.ParticleVelocities(ids=(0,))
+
+        for ctor in (espressomd.accumulators.MeanVarianceCalculator,
+                     espressomd.accumulators.TimeSeries):
+            for bad in (0, -1):
+                with self.assertRaisesRegex(RuntimeError, "delta_N must be >= 1"):
+                    ctor(obs=obs, delta_N=bad)
+            # runtime setter
+            acc = ctor(obs=obs, delta_N=1)
+            for bad in (0, -2):
+                with self.assertRaisesRegex(RuntimeError, "delta_N must be >= 1"):
+                    acc.delta_N = bad
+            self.assertEqual(acc.delta_N, 1)
+
 
 if __name__ == "__main__":
     ut.main()