median gives NaN if there are any NaN values

2 files changed, 18 insertions, 20 deletions

diff --git a/eval/src/tests/eval/aggr/aggr_test.cpp b/eval/src/tests/eval/aggr/aggr_test.cpp
index 612fb0a7630..b3e9c625fd9 100644
--- a/eval/src/tests/eval/aggr/aggr_test.cpp
+++ b/eval/src/tests/eval/aggr/aggr_test.cpp
@@ -87,21 +87,16 @@ TEST("require that MEDIAN aggregator works as expected") {
     aggr.next(200.0),  EXPECT_EQUAL(aggr.result(), 150.0);
 }
 
-TEST("require that MEDIAN aggregator ignores NaN values") {
+TEST("require that MEDIAN aggregator handles NaN values") {
     Stash stash;
     Aggregator &aggr = Aggregator::create(Aggr::MEDIAN, stash);
     double my_nan = std::numeric_limits<double>::quiet_NaN();
-    aggr.first(my_nan);
-    aggr.next(my_nan);
-    aggr.next(my_nan);
-    EXPECT_TRUE(std::isnan(aggr.result()));
-    aggr.next(10.0);
+    aggr.first(10.0);
     EXPECT_EQUAL(aggr.result(), 10.0);
     aggr.next(my_nan);
-    aggr.next(my_nan);
-    EXPECT_EQUAL(aggr.result(), 10.0);
+    EXPECT_TRUE(std::isnan(aggr.result()));
     aggr.next(20.0);
-    EXPECT_EQUAL(aggr.result(), 15.0);
+    EXPECT_TRUE(std::isnan(aggr.result()));
 }
 
 TEST("require that MIN aggregator works as expected") {
@@ -143,7 +138,8 @@ TEST("require that aggregator merge works") {
     EXPECT_EQUAL(aggr_merge<Median>({1,2},{3,4}), 2.5);
     EXPECT_EQUAL(aggr_merge<Median>({1,2},{3,4,5}), 3);
     EXPECT_EQUAL(aggr_merge<Median>({0,1,2},{3,4}), 2);
-    EXPECT_EQUAL(aggr_merge<Median>({my_nan,2,my_nan},{my_nan,4}), 3);
+    EXPECT_TRUE(std::isnan(aggr_merge<Median>({1,2,my_nan,3},{4,5})));
+    EXPECT_TRUE(std::isnan(aggr_merge<Median>({1,2,3},{4,my_nan,5})));
     EXPECT_EQUAL(aggr_merge<Min>({1,2},{3,4}), 1.0);
 }
 
diff --git a/eval/src/vespa/eval/eval/aggr.h b/eval/src/vespa/eval/eval/aggr.h
index b75b07422c1..f52c029eee5 100644
--- a/eval/src/vespa/eval/eval/aggr.h
+++ b/eval/src/vespa/eval/eval/aggr.h
@@ -127,24 +127,26 @@ private:
     std::vector<T> _seen;
 public:
     constexpr Median() : _seen() {}
-    constexpr Median(T value) : _seen() {
-        sample(value);
-    }
-    constexpr void sample(T value) {
-        if (!std::isnan(value)) {
-            _seen.push_back(value);
-        }
-    }
+    constexpr Median(T value) : _seen({value}) {}
+    constexpr void sample(T value) { _seen.push_back(value); }
     constexpr void merge(const Median &rhs) {
         for (T value: rhs._seen) {
-            sample(value);
+            _seen.push_back(value);
         }
     };
     constexpr T result() const {
         if (_seen.empty()) {
             return std::numeric_limits<T>::quiet_NaN();
         }
-        std::vector<T> tmp = _seen;
+        std::vector<T> tmp;
+        tmp.reserve(_seen.size());
+        for (T value: _seen) {
+            if (!std::isnan(value)) {
+                tmp.push_back(value);
+            } else {
+                return std::numeric_limits<T>::quiet_NaN();
+            }
+        }
         size_t n = (tmp.size() / 2);
         std::nth_element(tmp.begin(), tmp.begin() + n, tmp.end());
         T result = tmp[n]; // the nth element