Merge branch 'bartgol/eamxx/fix-fill-val-def' (PR #7557)

bartgol · web-flow · commit 982878e77caf · 2025-07-29T16:55:39.000-06:00
Changes:

- For floating point numbers, ensure it is ALWAYS the same number.
- Switch to an inline templated constexpr variable.
- Test behavior of fill_aware_combine
- Test behavior of field ops with a fill-value RHS

[non-BFB for certain EAMxx outputs]
diff --git a/components/eamxx/src/diagnostics/aodvis.cpp b/components/eamxx/src/diagnostics/aodvis.cpp
@@ -47,7 +47,7 @@ void AODVis::initialize_impl(const RunType /*run_type*/) {
   auto nondim = ekat::units::Units::nondimensional();
   const auto &grid_name =
       m_diagnostic_output.get_header().get_identifier().get_grid_name();
-  const auto var_fill_value = constants::DefaultFillValue<Real>().value;
+  const auto var_fill_value = constants::fill_value<Real>;
 
   m_mask_val = m_params.get<double>("mask_value", var_fill_value);
 
diff --git a/components/eamxx/src/diagnostics/atm_backtend.cpp b/components/eamxx/src/diagnostics/atm_backtend.cpp
@@ -63,7 +63,7 @@ void AtmBackTendDiag::init_timestep(const util::TimeStamp &start_of_step) {
 }
 
 void AtmBackTendDiag::compute_diagnostic_impl() {
-  Real var_fill_value = constants::DefaultFillValue<Real>().value;
+  Real var_fill_value = constants::fill_value<Real>;
   std::int64_t dt;
 
   const auto &f       = get_field_in(m_name);
diff --git a/components/eamxx/src/diagnostics/field_at_pressure_level.cpp b/components/eamxx/src/diagnostics/field_at_pressure_level.cpp
@@ -85,7 +85,7 @@ initialize_impl (const RunType /*run_type*/)
   // Add a field representing the mask as extra data to the diagnostic field.
   auto nondim = ekat::units::Units::nondimensional();
   const auto& gname = fid.get_grid_name();
-  m_mask_val = m_params.get<double>("mask_value",Real(constants::DefaultFillValue<float>::value));
+  m_mask_val = m_params.get<double>("mask_value",Real(constants::fill_value<double>));
 
 
   std::string mask_name = m_diag_name + " mask";
diff --git a/components/eamxx/src/diagnostics/tests/aodvis_test.cpp b/components/eamxx/src/diagnostics/tests/aodvis_test.cpp
@@ -31,7 +31,7 @@ TEST_CASE("aodvis") {
   using namespace ShortFieldTagsNames;
   using namespace ekat::units;
 
-  Real var_fill_value = constants::DefaultFillValue<Real>().value;
+  Real var_fill_value = constants::fill_value<Real>;
 
   Real some_limit = 0.0025;
 
diff --git a/components/eamxx/src/diagnostics/tests/atm_backtend_test.cpp b/components/eamxx/src/diagnostics/tests/atm_backtend_test.cpp
@@ -65,7 +65,7 @@ TEST_CASE("atm_backtend") {
   REQUIRE_THROWS(diag_factory.create("AtmBackTendDiag", comm,
                                      params));  // No 'tendency_name'
 
-  Real var_fill_value = constants::DefaultFillValue<Real>().value;
+  Real var_fill_value = constants::fill_value<Real>;
 
   // Set time for qc and randomize its values
   qc.get_header().get_tracking().update_time_stamp(t0);
diff --git a/components/eamxx/src/physics/nudging/eamxx_nudging_process_interface.cpp b/components/eamxx/src/physics/nudging/eamxx_nudging_process_interface.cpp
@@ -344,7 +344,7 @@ void Nudging::run_impl (const double dt)
     const auto fl = f.get_header().get_identifier().get_layout();
     const auto v  = f.get_view<Real**>();
 
-    Real var_fill_value = constants::DefaultFillValue<Real>().value;
+    Real var_fill_value = constants::fill_value<Real>;
     // Query the helper field for the fill value, if not present use default
     if (f.get_header().has_extra_data("mask_value")) {
       var_fill_value = f.get_header().get_extra_data<Real>("mask_value");
diff --git a/components/eamxx/src/share/field/field_impl.hpp b/components/eamxx/src/share/field/field_impl.hpp
@@ -559,9 +559,8 @@ update (const Field& x, const ST alpha, const ST beta)
       " - x layout: " + x_l.to_string() + "\n"
       " - y layout: " + y_l.to_string() + "\n");
 
-  // Determine if there is a FillValue that requires extra treatment.
-  bool use_fill = get_header().has_extra_data("mask_value") or
-                  x.get_header().has_extra_data("mask_value");
+  // Determine if the RHS has fill value entries that require extra treatment
+  bool use_fill = x.get_header().has_extra_data("mask_value");
 
   if (dt==DataType::IntType) {
     if (use_fill) {
@@ -609,18 +608,13 @@ update_impl (const Field& x, const ST alpha, const ST beta)
   const auto& layout = x.get_header().get_identifier().get_layout();
   const auto& dims = layout.dims();
 
-  ST fill_val = 0;
+  XST fill_val = 0;
   if constexpr (use_fill) {
-    if (get_header().has_extra_data("mask_value")) {
-      fill_val = get_header().get_extra_data<ST>("mask_value");
-    } else if (x.get_header().has_extra_data("mask_value")) {
-      fill_val = static_cast<ST>(x.get_header().get_extra_data<XST>("mask_value"));
-    } else {
-      EKAT_ERROR_MSG ("Error! Field::update_impl called with use_fill,\n"
-                      "       but neither *this nor x has mask_value extra data.\n"
-                      " - *this name: " + name() + "\n"
-                      " - x name: " + x.name() + "\n");
-    }
+    EKAT_REQUIRE_MSG (x.get_header().has_extra_data("mask_value"),
+        "Error! Field::update_impl called with use_fill, but x does not have mask_value extra data.\n"
+        " - *this name: " + name() + "\n"
+        " - x name: " + x.name() + "\n");
+    fill_val = x.get_header().get_extra_data<XST>("mask_value");
   }
 
   // Must handle the case where one of the two views is strided (or both)
diff --git a/components/eamxx/src/share/field/field_impl_details.hpp b/components/eamxx/src/share/field/field_impl_details.hpp
@@ -114,7 +114,7 @@ struct CombineViewsHelper {
 template<CombineMode CM, bool use_fill, typename LhsView, typename RhsView, typename ST>
 void
 cvh (LhsView lhs, RhsView rhs,
-     ST alpha, ST beta, typename LhsView::traits::value_type fill_val,
+     ST alpha, ST beta, typename RhsView::traits::value_type fill_val,
      const std::vector<int>& dims)
 {
   CombineViewsHelper <CM, use_fill, LhsView, RhsView,  ST> helper;
diff --git a/components/eamxx/src/share/io/eamxx_scorpio_interface.cpp b/components/eamxx/src/share/io/eamxx_scorpio_interface.cpp
@@ -159,6 +159,7 @@ int nctype () {
   } else {
     EKAT_ERROR_MSG ("Error! Unrecognized/unsupported data type.\n");
   }
+  return PIO_NAT;
 }
 
 template<typename T>
diff --git a/components/eamxx/src/share/io/scorpio_output.hpp b/components/eamxx/src/share/io/scorpio_output.hpp
@@ -219,7 +219,7 @@ class AtmosphereOutput
   //   NetCDF: Numeric conversion not representable
   // Also, by default, don't pick max float, to avoid any overflow if the value
   // is used inside other calculation and/or remap.
-  float m_fill_value = constants::DefaultFillValue<float>().value;
+  float m_fill_value = constants::fill_value<float>;
 
   bool m_add_time_dim;
   bool m_track_avg_cnt = false;
diff --git a/components/eamxx/src/share/io/tests/io_basic.cpp b/components/eamxx/src/share/io/tests/io_basic.cpp
@@ -274,7 +274,7 @@ void read (const std::string& avg_type, const std::string& freq_units,
   // Check that the expected metadata was appropriately set for each variable
   for (const auto& fn: fnames) {
     auto att_fill = scorpio::get_attribute<float>(filename,fn,"_FillValue");
-    REQUIRE(att_fill==constants::DefaultFillValue<float>().value);
+    REQUIRE(att_fill==constants::fill_value<Real>);
 
     auto att_str = scorpio::get_attribute<std::string>(filename,fn,"test");
     REQUIRE (att_str==fn);
diff --git a/components/eamxx/src/share/io/tests/io_filled.cpp b/components/eamxx/src/share/io/tests/io_filled.cpp
@@ -27,7 +27,7 @@
 namespace scream {
 
 constexpr int num_output_steps = 5;
-constexpr Real FillValue = constants::DefaultFillValue<float>().value;
+constexpr Real FillValue = constants::fill_value<Real>;
 constexpr Real fill_threshold = 0.5;
 
 void set (const Field& f, const double v) {
@@ -255,7 +255,7 @@ void read (const std::string& avg_type, const std::string& freq_units,
   for (const auto& fn: fnames) {
     // NOTE: use float, since default fp_precision for I/O is 'single'
     auto att_fill = scorpio::get_attribute<float>(filename,fn,"_FillValue");
-    REQUIRE(att_fill==constants::DefaultFillValue<float>().value);
+    REQUIRE(att_fill==constants::fill_value<Real>);
   }
 }
 
diff --git a/components/eamxx/src/share/io/tests/io_remap_test.cpp b/components/eamxx/src/share/io/tests/io_remap_test.cpp
@@ -283,7 +283,7 @@ TEST_CASE("io_remap_test","io_remap_test")
   {
     // Note, the vertical remapper defaults to a mask value of std numeric limits scaled by 0.1;
     const float mask_val = vert_remap_control.isParameter("Fill Value")
-                         ? vert_remap_control.get<double>("Fill Value") : constants::DefaultFillValue<float>().value;
+                         ? vert_remap_control.get<double>("Fill Value") : constants::fill_value<Real>;
     print ("    -> vertical remap ... \n",io_comm);
     auto gm_vert   = get_test_gm(io_comm,ncols_src,nlevs_tgt);
     auto grid_vert = gm_vert->get_grid("point_grid");
@@ -353,7 +353,7 @@ TEST_CASE("io_remap_test","io_remap_test")
   {
     // Note, the vertical remapper defaults to a mask value of std numeric limits scaled by 0.1;
     const float mask_val = horiz_remap_control.isParameter("Fill Value")
-                         ? horiz_remap_control.get<double>("Fill Value") : constants::DefaultFillValue<float>().value;
+                         ? horiz_remap_control.get<double>("Fill Value") : constants::fill_value<Real>;
     print ("    -> horizontal remap ... \n",io_comm);
     auto gm_horiz   = get_test_gm(io_comm,ncols_tgt,nlevs_src);
     auto grid_horiz = gm_horiz->get_grid("point_grid");
@@ -439,7 +439,7 @@ TEST_CASE("io_remap_test","io_remap_test")
   //                                ---  Vertical + Horizontal Remapping ---
   {
     const float mask_val = vert_horiz_remap_control.isParameter("Fill Value")
-                         ? vert_horiz_remap_control.get<double>("Fill Value") : constants::DefaultFillValue<float>().value;
+                         ? vert_horiz_remap_control.get<double>("Fill Value") : constants::fill_value<Real>;
     print ("    -> vertical + horizontal remap ... \n",io_comm);
     auto gm_vh   = get_test_gm(io_comm,ncols_tgt,nlevs_tgt);
     auto grid_vh = gm_vh->get_grid("point_grid");
diff --git a/components/eamxx/src/share/io/tests/output_restart.cpp b/components/eamxx/src/share/io/tests/output_restart.cpp
@@ -27,7 +27,7 @@
 
 namespace scream {
 
-constexpr Real FillValue = constants::DefaultFillValue<float>().value;
+constexpr Real FillValue = constants::fill_value<Real>;
 
 std::shared_ptr<FieldManager>
 get_test_fm(const std::shared_ptr<const AbstractGrid>& grid);
diff --git a/components/eamxx/src/share/tests/CMakeLists.txt b/components/eamxx/src/share/tests/CMakeLists.txt
@@ -5,6 +5,9 @@ if (NOT SCREAM_ONLY_GENERATE_BASELINES)
   # Test utils
   CreateUnitTest(utils "utils_tests.cpp")
 
+  # Test combine operations
+  CreateUnitTest(combine_ops "combine_ops.cpp")
+
   # Test column ops
   CreateUnitTest(column_ops "column_ops.cpp")
 
diff --git a/components/eamxx/src/share/tests/column_ops.cpp b/components/eamxx/src/share/tests/column_ops.cpp
@@ -7,37 +7,6 @@
 
 namespace {
 
-TEST_CASE ("combine_ops") {
-  using namespace scream;
-  using pack_type = ekat::Pack<Real,SCREAM_PACK_SIZE>;
-
-  constexpr auto Replace      = CombineMode::Replace;
-  constexpr auto Update       = CombineMode::Update;
-  constexpr auto Multiply     = CombineMode::Multiply;
-  constexpr auto Divide       = CombineMode::Divide;
-
-  const pack_type two (2.0);
-  const pack_type four (4.0);
-  const pack_type six (6.0);
-  const pack_type ten (10.0);
-  pack_type x;
-
-  x = two;
-  combine<Replace>(two,x,1,0);
-  REQUIRE ( (x==two).all() );
-
-  combine<Update>(two,x,2.0,1.0);
-  REQUIRE ( (x==six).all() );
-
-  x = two;
-  combine<Multiply>(two,x,1,1);
-  REQUIRE ( (x==four).all() );
-
-  x = four;
-  combine<Divide>(two,x,1,1);
-  REQUIRE ( (x==two).all() );
-}
-
 TEST_CASE("column_ops_ps_1") {
   using namespace scream;
   using device_type = DefaultDevice;
diff --git a/components/eamxx/src/share/tests/combine_ops.cpp b/components/eamxx/src/share/tests/combine_ops.cpp
@@ -0,0 +1,74 @@
+#include <catch2/catch.hpp>
+
+#include "share/util/eamxx_combine_ops.hpp"
+#include "share/util/eamxx_universal_constants.hpp"
+#include "share/eamxx_types.hpp"
+
+#include "ekat/ekat_pack.hpp"
+
+#include <iomanip>
+
+namespace {
+
+TEST_CASE ("combine_ops") {
+  using namespace scream;
+  using pack_type = ekat::Pack<Real,SCREAM_PACK_SIZE>;
+
+  constexpr auto Replace   = CombineMode::Replace;
+  constexpr auto Update    = CombineMode::Update;
+  constexpr auto Multiply  = CombineMode::Multiply;
+  constexpr auto Divide    = CombineMode::Divide;
+  constexpr auto Max       = CombineMode::Max;
+  constexpr auto Min       = CombineMode::Min;
+  constexpr auto fv_val    = constants::fill_value<Real>;
+
+  const pack_type two (2.0);
+  const pack_type four (4.0);
+  const pack_type six (6.0);
+  const pack_type ten (10.0);
+  const pack_type fv (constants::fill_value<Real>);
+  const pack_type fv_times_ten (constants::fill_value<Real> * 10);
+
+  pack_type x;
+
+  x = two;
+  combine<Replace>(two,x,1,0);
+  REQUIRE ( (x==two).all() );
+
+  combine<Update>(two,x,2.0,1.0);
+  REQUIRE ( (x==six).all() );
+  fill_aware_combine<Update>(fv,x,fv_val,2.0,1.0);
+  if (not (x==six).all() ) {
+    std::cout << "x: " << x << "\n";
+    std::cout << " x[0]: " << std::setprecision(18) << x[0] << "\n";
+    std::cout << "fv[0]: " << std::setprecision(18) << fv[0] << "\n";
+  }
+  REQUIRE ( (x==six).all() );
+
+  x = two;
+  combine<Multiply>(two,x,1,1);
+  REQUIRE ( (x==four).all() );
+  fill_aware_combine<Multiply>(fv,x,fv_val,1,1);
+  REQUIRE ( (x==four).all() );
+
+  x = four;
+  combine<Divide>(two,x,1,1);
+  REQUIRE ( (x==two).all() );
+  fill_aware_combine<Divide>(fv,x,fv_val,1,1);
+  REQUIRE ( (x==two).all() );
+
+  x = two;
+  combine<Max>(four,x,1,1);
+  REQUIRE ( (x==four).all() );
+  fill_aware_combine<Max>(fv,x,fv_val,1,1);
+  REQUIRE ( (x==four).all() );
+
+  x = four;
+  combine<Min>(two,x,1,1);
+  REQUIRE ( (x==two).all() );
+  x = fv_times_ten;
+  fill_aware_combine<Min>(fv,x,fv_val,1,1);
+  REQUIRE ( (x==fv_times_ten).all() );
+}
+
+} // anonymous namespace
diff --git a/components/eamxx/src/share/tests/field_tests.cpp b/components/eamxx/src/share/tests/field_tests.cpp
@@ -887,6 +887,13 @@ TEST_CASE ("update") {
       Field f4 = two.clone();
       f4.min(f3);
       REQUIRE (views_are_equal(f3, f4));
+
+      // Check that updating with rhs==fill_value ignores the rhs
+      f3.deep_copy(constants::fill_value<Real>);
+      f3.get_header().set_extra_data("mask_value",constants::fill_value<Real>);
+      f2.deep_copy(1.0);
+      f2.max(f3);
+      REQUIRE (views_are_equal(f2,one));
     }
 
     SECTION ("int") {
@@ -904,6 +911,13 @@ TEST_CASE ("update") {
       Field f4 = two.clone();
       f4.min(f3);
       REQUIRE (views_are_equal(f3, f4));
+
+      // Check that updating with rhs==fill_value ignores the rhs
+      f3.deep_copy(constants::fill_value<int>);
+      f3.get_header().set_extra_data("mask_value",constants::fill_value<int>);
+      f2.deep_copy(1);
+      f2.max(f3);
+      REQUIRE (views_are_equal(f2,one));
     }
   }
 
@@ -949,6 +963,19 @@ TEST_CASE ("update") {
       // Same, but we discard current content of f3
       f3.update(f_real,2,0);
       REQUIRE (views_are_equal(f3,f2));
+
+      // Check that updating with rhs==fill_value ignores the rhs
+      Field one = f_real.clone();
+      one.deep_copy(1.0);
+
+      f3.deep_copy(constants::fill_value<Real>);
+      f3.get_header().set_extra_data("mask_value",constants::fill_value<Real>);
+      f2.deep_copy(1.0);
+      f2.update(f3,1,1);
+      if (not views_are_equal(f2,one)) {
+        print_field_hyperslab(f2);
+      }
+      REQUIRE (views_are_equal(f2,one));
     }
 
     SECTION ("int") {
@@ -968,6 +995,16 @@ TEST_CASE ("update") {
       // Same, but we discard current content of f3
       f3.update(f_int,2,0);
       REQUIRE (views_are_equal(f3,f2));
+
+      // Check that updating with rhs==fill_value ignores the rhs
+      Field one = f_int.clone();
+      one.deep_copy(1);
+
+      f3.deep_copy(constants::fill_value<int>);
+      f3.get_header().set_extra_data("mask_value",constants::fill_value<int>);
+      f2.deep_copy(1);
+      f2.update(f3,1,1);
+      REQUIRE (views_are_equal(f2,one));
     }
   }
 }
diff --git a/components/eamxx/src/share/tests/utils_tests.cpp b/components/eamxx/src/share/tests/utils_tests.cpp
@@ -7,6 +7,16 @@
 #include "share/util/eamxx_setup_random_test.hpp"
 #include "share/eamxx_config.hpp"
 
+TEST_CASE("fill_value") {
+  using namespace scream::constants;
+
+  // Ensure we have the SAME numerical value for both float and double
+  auto fv_d = fill_value<double>;
+  auto fv_f = fill_value<float>;
+
+  REQUIRE (fv_d==fv_f);
+}
+
 TEST_CASE("contiguous_superset") {
   using namespace scream;
 
diff --git a/components/eamxx/src/share/util/eamxx_combine_ops.hpp b/components/eamxx/src/share/util/eamxx_combine_ops.hpp
diff --git a/components/eamxx/src/share/util/eamxx_universal_constants.hpp b/components/eamxx/src/share/util/eamxx_universal_constants.hpp

Original file line number	Diff line number	Diff line change
`@@ -63,7 +63,7 @@ void AtmBackTendDiag::init_timestep(const util::TimeStamp &start_of_step) {`
`63`	`63`	`}`
`64`	`64`
`65`	`65`	`void AtmBackTendDiag::compute_diagnostic_impl() {`
`66`		`- Real var_fill_value = constants::DefaultFillValue<Real>().value;`
	`66`	`+ Real var_fill_value = constants::fill_value<Real>;`
`67`	`67`	`std::int64_t dt;`
`68`	`68`
`69`	`69`	`const auto &f = get_field_in(m_name);`
Original file line number	Diff line number	Diff line change
`@@ -114,7 +114,7 @@ struct CombineViewsHelper {`
`114`	`114`	`template<CombineMode CM, bool use_fill, typename LhsView, typename RhsView, typename ST>`
`115`	`115`	`void`
`116`	`116`	`cvh (LhsView lhs, RhsView rhs,`
`117`		`- ST alpha, ST beta, typename LhsView::traits::value_type fill_val,`
	`117`	`+ ST alpha, ST beta, typename RhsView::traits::value_type fill_val,`
`118`	`118`	`const std::vector<int>& dims)`
`119`	`119`	`{`
`120`	`120`	`CombineViewsHelper <CM, use_fill, LhsView, RhsView, ST> helper;`
Original file line number	Diff line number	Diff line change
`@@ -159,6 +159,7 @@ int nctype () {`
`159`	`159`	`} else {`
`160`	`160`	`EKAT_ERROR_MSG ("Error! Unrecognized/unsupported data type.\n");`
`161`	`161`	`}`
	`162`	`+ return PIO_NAT;`
`162`	`163`	`}`
`163`	`164`
`164`	`165`	`template<typename T>`