From 1fc7e0b69bd2612b9cf16aaf1ad6406ca9a5b8b8 Mon Sep 17 00:00:00 2001
From: Stephane Del Pino <stephane.delpino44@gmail.com>
Date: Sun, 21 Apr 2024 20:17:05 +0200
Subject: [PATCH] Fix max of array types in the case of all non-positive values

Thanks to Philippe for noticing it.

This was a classic misused of std::numeric_limits<T>::min()...
---
 src/mesh/ItemArrayUtils.hpp             |  2 +-
 src/mesh/ItemValueUtils.hpp             |  2 +-
 src/mesh/SubItemArrayPerItemUtils.hpp   |  2 +-
 src/mesh/SubItemValuePerItemUtils.hpp   |  2 +-
 src/utils/Array.hpp                     |  2 +-
 tests/test_Array.cpp                    | 97 +++++++++++++++++++++++++
 tests/test_ItemArrayUtils.cpp           | 60 +++++++++++++++
 tests/test_ItemValueUtils.cpp           | 28 +++++++
 tests/test_SubItemArrayPerItemUtils.cpp | 33 +++++++++
 tests/test_SubItemValuePerItemUtils.cpp | 30 ++++++++
 10 files changed, 253 insertions(+), 5 deletions(-)

diff --git a/src/mesh/ItemArrayUtils.hpp b/src/mesh/ItemArrayUtils.hpp
index 0d90f77af..cb8ee339b 100644
--- a/src/mesh/ItemArrayUtils.hpp
+++ b/src/mesh/ItemArrayUtils.hpp
@@ -165,7 +165,7 @@ max(const ItemArray<DataType, item_type, ConnectivityPtr>& item_array)
     void
     init(data_type& value) const
     {
-      value = std::numeric_limits<data_type>::min();
+      value = std::numeric_limits<data_type>::lowest();
     }
 
     PUGS_INLINE
diff --git a/src/mesh/ItemValueUtils.hpp b/src/mesh/ItemValueUtils.hpp
index e64f2e1bd..031caa640 100644
--- a/src/mesh/ItemValueUtils.hpp
+++ b/src/mesh/ItemValueUtils.hpp
@@ -156,7 +156,7 @@ max(const ItemValue<DataType, item_type, ConnectivityPtr>& item_value)
     void
     init(data_type& value) const
     {
-      value = std::numeric_limits<data_type>::min();
+      value = std::numeric_limits<data_type>::lowest();
     }
 
     PUGS_INLINE
diff --git a/src/mesh/SubItemArrayPerItemUtils.hpp b/src/mesh/SubItemArrayPerItemUtils.hpp
index aec08ef84..70b835ae3 100644
--- a/src/mesh/SubItemArrayPerItemUtils.hpp
+++ b/src/mesh/SubItemArrayPerItemUtils.hpp
@@ -170,7 +170,7 @@ max(const SubItemArrayPerItem<DataType, ItemOfItem, ConnectivityPtr>& sub_item_a
     void
     init(data_type& value) const
     {
-      value = std::numeric_limits<data_type>::min();
+      value = std::numeric_limits<data_type>::lowest();
     }
 
     PUGS_INLINE
diff --git a/src/mesh/SubItemValuePerItemUtils.hpp b/src/mesh/SubItemValuePerItemUtils.hpp
index 575c39c52..cc0c941f7 100644
--- a/src/mesh/SubItemValuePerItemUtils.hpp
+++ b/src/mesh/SubItemValuePerItemUtils.hpp
@@ -166,7 +166,7 @@ max(const SubItemValuePerItem<DataType, ItemOfItem, ConnectivityPtr>& sub_item_v
     void
     init(data_type& value) const
     {
-      value = std::numeric_limits<data_type>::min();
+      value = std::numeric_limits<data_type>::lowest();
     }
 
     PUGS_INLINE
diff --git a/src/utils/Array.hpp b/src/utils/Array.hpp
index 2f0146e2b..a08453ba7 100644
--- a/src/utils/Array.hpp
+++ b/src/utils/Array.hpp
@@ -355,7 +355,7 @@ max(const Array<DataType>& array)
     void
     init(data_type& value) const
     {
-      value = std::numeric_limits<data_type>::min();
+      value = std::numeric_limits<data_type>::lowest();
     }
 
     PUGS_INLINE
diff --git a/tests/test_Array.cpp b/tests/test_Array.cpp
index 4a447f70a..28c018cce 100644
--- a/tests/test_Array.cpp
+++ b/tests/test_Array.cpp
@@ -324,6 +324,103 @@ TEST_CASE("Array", "[utils]")
     }
   }
 
+  SECTION("checking for floating Array min/max")
+  {
+    SECTION("Min")
+    {
+      Array<double> b(10);
+      b[0] = 13;
+      b[1] = 1;
+      b[2] = 8;
+      b[3] = -3;
+      b[4] = 23;
+      b[5] = -1;
+      b[6] = 13;
+      b[7] = 0;
+      b[8] = 12;
+      b[9] = 9;
+
+      REQUIRE(min(b) == -3);
+
+      b.fill(0);
+      REQUIRE(min(b) == 0);
+
+      b[0] = -13;
+      b[1] = -1;
+      b[2] = -8;
+      b[3] = -3;
+      b[4] = -23;
+      b[5] = -1;
+      b[6] = -13;
+      b[7] = 0;
+      b[8] = -12;
+      b[9] = -9;
+
+      REQUIRE(min(b) == -23);
+
+      b[0] = 13;
+      b[1] = 1;
+      b[2] = 8;
+      b[3] = 3;
+      b[4] = 23;
+      b[5] = 1;
+      b[6] = 13;
+      b[7] = 0;
+      b[8] = 12;
+      b[9] = 9;
+
+      REQUIRE(min(b) == 0);
+
+      b[7] = 3;
+      REQUIRE(min(b) == 1);
+    }
+
+    SECTION("Max")
+    {
+      Array<double> b(10);
+      b[0] = 13;
+      b[1] = 1;
+      b[2] = 8;
+      b[3] = -3;
+      b[4] = 23;
+      b[5] = -1;
+      b[6] = 13;
+      b[7] = 0;
+      b[8] = 12;
+      b[9] = 9;
+
+      REQUIRE(max(b) == 23);
+
+      b[0] = -13;
+      b[1] = -12;
+      b[2] = -8;
+      b[3] = -3;
+      b[4] = -23;
+      b[5] = -1;
+      b[6] = -13;
+      b[7] = -10;
+      b[8] = -12;
+      b[9] = -9;
+
+      REQUIRE(max(b) == -1);
+
+      b.fill(-13);
+      REQUIRE(max(b) == -13);
+
+      b[0] = 13;
+      b[1] = 12;
+      b[2] = 8;
+      b[3] = 3;
+      b[4] = 23;
+      b[5] = 1;
+      b[6] = 13;
+      b[7] = 10;
+      b[8] = 12;
+      b[9] = 9;
+      REQUIRE(max(b) == 23);
+    }
+  }
+
   SECTION("reproducible floating point sum")
   {
     auto direct_sum = [](auto array) {
diff --git a/tests/test_ItemArrayUtils.cpp b/tests/test_ItemArrayUtils.cpp
index 8d0484284..0237b90da 100644
--- a/tests/test_ItemArrayUtils.cpp
+++ b/tests/test_ItemArrayUtils.cpp
@@ -1028,6 +1028,66 @@ TEST_CASE("ItemArrayUtils", "[mesh]")
         }
       }
     }
+
+    SECTION("max for negative double values")
+    {
+      std::array mesh_list = MeshDataBaseForTests::get().all3DMeshes();
+
+      for (const auto& named_mesh : mesh_list) {
+        SECTION(named_mesh.name())
+        {
+          auto mesh_3d_v = named_mesh.mesh();
+          auto mesh_3d   = mesh_3d_v->get<Mesh<3>>();
+
+          const Connectivity<3>& connectivity = mesh_3d->connectivity();
+
+          CellArray<double> cell_array{connectivity, 2};
+          cell_array.fill(std::numeric_limits<double>::max());
+
+          auto cell_is_owned = connectivity.cellIsOwned();
+          parallel_for(
+            mesh_3d->numberOfCells(), PUGS_LAMBDA(CellId cell_id) {
+              if (cell_is_owned[cell_id]) {
+                for (size_t i = 0; i < cell_array.sizeOfArrays(); ++i) {
+                  cell_array[cell_id][i] = -parallel::rank() - 10 + i;
+                }
+              }
+            });
+
+          REQUIRE(max(cell_array) == -parallel::size() - 10);
+        }
+      }
+    }
+
+    SECTION("max for negative integral values")
+    {
+      std::array mesh_list = MeshDataBaseForTests::get().all3DMeshes();
+
+      for (const auto& named_mesh : mesh_list) {
+        SECTION(named_mesh.name())
+        {
+          auto mesh_3d_v = named_mesh.mesh();
+          auto mesh_3d   = mesh_3d_v->get<Mesh<3>>();
+
+          const Connectivity<3>& connectivity = mesh_3d->connectivity();
+
+          CellArray<int> cell_array{connectivity, 2};
+          cell_array.fill(std::numeric_limits<int>::max());
+
+          auto cell_is_owned = connectivity.cellIsOwned();
+          parallel_for(
+            mesh_3d->numberOfCells(), PUGS_LAMBDA(CellId cell_id) {
+              if (cell_is_owned[cell_id]) {
+                for (size_t i = 0; i < cell_array.sizeOfArrays(); ++i) {
+                  cell_array[cell_id][i] = -2 * parallel::size() + parallel::rank() - 10 + i;
+                }
+              }
+            });
+
+          REQUIRE(max(cell_array) == -static_cast<int>(parallel::size() + 10));
+        }
+      }
+    }
   }
 
   SECTION("sum")
diff --git a/tests/test_ItemValueUtils.cpp b/tests/test_ItemValueUtils.cpp
index f80819f2c..149dcbfa6 100644
--- a/tests/test_ItemValueUtils.cpp
+++ b/tests/test_ItemValueUtils.cpp
@@ -240,6 +240,34 @@ TEST_CASE("ItemValueUtils", "[mesh]")
         }
       }
     }
+
+    SECTION("max of all negative values")
+    {
+      std::array mesh_list = MeshDataBaseForTests::get().all3DMeshes();
+
+      for (const auto& named_mesh : mesh_list) {
+        SECTION(named_mesh.name())
+        {
+          auto mesh_3d_v = named_mesh.mesh();
+          auto mesh_3d   = mesh_3d_v->get<Mesh<3>>();
+
+          const Connectivity<3>& connectivity = mesh_3d->connectivity();
+
+          CellValue<float> cell_value{connectivity};
+          cell_value.fill(std::numeric_limits<float>::max());
+
+          auto cell_is_owned = connectivity.cellIsOwned();
+          parallel_for(
+            mesh_3d->numberOfCells(), PUGS_LAMBDA(CellId cell_id) {
+              if (cell_is_owned[cell_id]) {
+                cell_value[cell_id] = -2 * parallel::size() + parallel::rank() + 1;
+              }
+            });
+
+          REQUIRE(max(cell_value) == -parallel::size());
+        }
+      }
+    }
   }
 
   SECTION("sum")
diff --git a/tests/test_SubItemArrayPerItemUtils.cpp b/tests/test_SubItemArrayPerItemUtils.cpp
index 1ff8bb17f..ce3bb6265 100644
--- a/tests/test_SubItemArrayPerItemUtils.cpp
+++ b/tests/test_SubItemArrayPerItemUtils.cpp
@@ -283,6 +283,39 @@ TEST_CASE("SubItemArrayPerItemUtils", "[mesh]")
         }
       }
     }
+
+    SECTION("max of only negative values")
+    {
+      std::array mesh_list = MeshDataBaseForTests::get().all3DMeshes();
+
+      for (const auto& named_mesh : mesh_list) {
+        SECTION(named_mesh.name())
+        {
+          auto mesh_3d = named_mesh.mesh()->get<Mesh<3>>();
+
+          const Connectivity<3>& connectivity = mesh_3d->connectivity();
+
+          NodeArrayPerEdge<double> node_array_per_edge{connectivity, 3};
+          node_array_per_edge.fill(std::numeric_limits<double>::max());
+
+          auto edge_is_owned = connectivity.edgeIsOwned();
+
+          parallel_for(
+            connectivity.numberOfEdges(), PUGS_LAMBDA(EdgeId edge_id) {
+              if (edge_is_owned[edge_id]) {
+                auto edge_table = node_array_per_edge.itemTable(edge_id);
+                for (size_t i = 0; i < edge_table.numberOfRows(); ++i) {
+                  for (size_t j = 0; j < edge_table.numberOfColumns(); ++j) {
+                    edge_table(i, j) = -10. - parallel::size() + parallel::rank() - i - j;
+                  }
+                }
+              }
+            });
+
+          REQUIRE(max(node_array_per_edge) == -11.);
+        }
+      }
+    }
   }
 
   SECTION("sum")
diff --git a/tests/test_SubItemValuePerItemUtils.cpp b/tests/test_SubItemValuePerItemUtils.cpp
index 69214c1c9..fd84ad62e 100644
--- a/tests/test_SubItemValuePerItemUtils.cpp
+++ b/tests/test_SubItemValuePerItemUtils.cpp
@@ -267,6 +267,36 @@ TEST_CASE("SubItemValuePerItemUtils", "[mesh]")
         }
       }
     }
+
+    SECTION("max for all negative values")
+    {
+      std::array mesh_list = MeshDataBaseForTests::get().all2DMeshes();
+
+      for (const auto& named_mesh : mesh_list) {
+        SECTION(named_mesh.name())
+        {
+          auto mesh_2d = named_mesh.mesh()->get<Mesh<2>>();
+
+          const Connectivity<2>& connectivity = mesh_2d->connectivity();
+
+          EdgeValuePerCell<double> edge_value_per_cell{connectivity};
+          edge_value_per_cell.fill(std::numeric_limits<double>::max());
+
+          auto cell_is_owned = connectivity.cellIsOwned();
+          parallel_for(
+            connectivity.numberOfCells(), PUGS_LAMBDA(CellId cell_id) {
+              if (cell_is_owned[cell_id]) {
+                auto cell_array = edge_value_per_cell.itemArray(cell_id);
+                for (size_t i = 0; i < cell_array.size(); ++i) {
+                  cell_array[i] = -1. * parallel::size() - 10 + parallel::rank() - i;
+                }
+              }
+            });
+
+          REQUIRE(max(edge_value_per_cell) == -11);
+        }
+      }
+    }
   }
 
   SECTION("sum")
-- 
GitLab