diff --git a/extern/test_bmi_c/src/bmi_test_bmi_c.c b/extern/test_bmi_c/src/bmi_test_bmi_c.c index 522fe4651d..187ca666cd 100644 --- a/extern/test_bmi_c/src/bmi_test_bmi_c.c +++ b/extern/test_bmi_c/src/bmi_test_bmi_c.c @@ -21,7 +21,7 @@ static const char *output_var_locations[OUTPUT_VAR_NAME_COUNT] = { "node", "node // Don't forget to update Get_value/Get_value_at_indices (and setter) implementation if these are adjusted static const char *input_var_names[INPUT_VAR_NAME_COUNT] = { "INPUT_VAR_1", "INPUT_VAR_2" }; static const char *input_var_types[INPUT_VAR_NAME_COUNT] = { "double", "double" }; -static const char *input_var_units[INPUT_VAR_NAME_COUNT] = { "m", "m/s" }; +static const char *input_var_units[INPUT_VAR_NAME_COUNT] = { "m", "Pa" }; static const int input_var_item_count[INPUT_VAR_NAME_COUNT] = { 1, 1 }; static const char *input_var_grids[INPUT_VAR_NAME_COUNT] = { 0, 0 }; static const char *input_var_locations[INPUT_VAR_NAME_COUNT] = { "node", "node" }; diff --git a/extern/test_bmi_cpp/include/test_bmi_cpp.hpp b/extern/test_bmi_cpp/include/test_bmi_cpp.hpp index c15896def2..4b2c31a429 100644 --- a/extern/test_bmi_cpp/include/test_bmi_cpp.hpp +++ b/extern/test_bmi_cpp/include/test_bmi_cpp.hpp @@ -172,8 +172,8 @@ class TestBmiCpp : public bmi::Bmi { std::vector input_var_types = { "double", "double" }; std::vector output_var_types = { "double", "double" }; std::vector model_var_types = {}; - std::vector input_var_units = { "m", "m" }; - std::vector output_var_units = { "m", "m/s", "m", "m" }; + std::vector input_var_units = { "mm/s", "Pa" }; + std::vector output_var_units = { "mm/s", "m/s", "m", "m" }; std::vector model_var_units = {}; std::vector input_var_locations = { "node", "node" }; std::vector output_var_locations = { "node", "node" }; diff --git a/extern/test_bmi_fortran/src/bmi_test_bmi_fortran.f90 b/extern/test_bmi_fortran/src/bmi_test_bmi_fortran.f90 index 08a08aa10e..e6c07a5631 100644 --- a/extern/test_bmi_fortran/src/bmi_test_bmi_fortran.f90 +++ b/extern/test_bmi_fortran/src/bmi_test_bmi_fortran.f90 @@ -123,10 +123,10 @@ module bmitestbmi integer :: input_grid(4) = [0, 0, 0, 1] character (len=BMI_MAX_UNITS_NAME) :: & - output_units(6) = [character(BMI_MAX_UNITS_NAME):: 'm', 'm', 's', 'm', 'm', 'm'] + output_units(6) = [character(BMI_MAX_UNITS_NAME):: 'mm s^-1', 'm', 's', 'm', 'm', 'm'] character (len=BMI_MAX_UNITS_NAME) :: & - input_units(4) = [character(BMI_MAX_UNITS_NAME):: 'm', 'm', 's', 'm'] + input_units(4) = [character(BMI_MAX_UNITS_NAME):: 'mm s^-1', 'Pa', 'K', 'mm s^-1'] character (len=BMI_MAX_LOCATION_NAME) :: & output_location(6) = [character(BMI_MAX_LOCATION_NAME):: 'node', 'node', 'node', 'node', 'node', 'node'] diff --git a/extern/test_bmi_py/bmi_model.py b/extern/test_bmi_py/bmi_model.py index 0fbb93e1b7..efbd791dbd 100755 --- a/extern/test_bmi_py/bmi_model.py +++ b/extern/test_bmi_py/bmi_model.py @@ -73,12 +73,12 @@ def __init__(self): # since the input variable names could come from any forcing... #------------------------------------------------------ #_var_name_map_long_first = { - _var_name_units_map = {'INPUT_VAR_1':['INPUT_VAR_1','-'], - 'INPUT_VAR_2':['INPUT_VAR_2','-'], - 'OUTPUT_VAR_1':['OUTPUT_VAR_1','-'], - 'OUTPUT_VAR_2':['OUTPUT_VAR_2','-'], + _var_name_units_map = {'INPUT_VAR_1':['INPUT_VAR_1','mm/s'], + 'INPUT_VAR_2':['INPUT_VAR_2','Pa'], + 'OUTPUT_VAR_1':['OUTPUT_VAR_1','m'], + 'OUTPUT_VAR_2':['OUTPUT_VAR_2','mm/s'], 'OUTPUT_VAR_3':['OUTPUT_VAR_3','-'], - 'GRID_VAR_1':['OUTPUT_VAR_1','-'], + 'GRID_VAR_1':['OUTPUT_VAR_1','mm/s'], 'GRID_VAR_2':['GRID_VAR_2','-'], } @@ -609,4 +609,4 @@ def _parse_config(self, cfg): pass # Add more config parsing if necessary - return cfg \ No newline at end of file + return cfg diff --git a/include/forcing/CsvPerFeatureForcingProvider.hpp b/include/forcing/CsvPerFeatureForcingProvider.hpp index b0d2f85903..b8d05cb338 100644 --- a/include/forcing/CsvPerFeatureForcingProvider.hpp +++ b/include/forcing/CsvPerFeatureForcingProvider.hpp @@ -170,13 +170,13 @@ class CsvPerFeatureForcingProvider : public data_access::GenericDataProvider try { return UnitsHelper::get_converted_value(available_forcings_units[output_name], value, output_units); } - catch (const std::runtime_error& e){ - #ifndef UDUNITS_QUIET - std::stringstream ss; - ss <<"WARN: Unit conversion unsuccessful - Returning unconverted value! (\""<(wkf->second) : units; + auto wkf_name = std::get<0>(wkf->second); + LOG("CsvProvider has well-known name '" + wkf_name + "' for variable '" + var_name + "' with units '" + units + "'", LogLevel::DEBUG); available_forcings.push_back(var_name); // Allow lookup by non-canonical name available_forcings_units[var_name] = units; // Allow lookup of units by non-canonical name - var_name = std::get<0>(wkf->second); // Use the CSDMS name from here on + var_name = wkf_name; // Use the CSDMS name from here on } forcing_vectors[var_name] = {}; diff --git a/include/forcing/DataProvider.hpp b/include/forcing/DataProvider.hpp index 32cbb36029..2cf4573f08 100644 --- a/include/forcing/DataProvider.hpp +++ b/include/forcing/DataProvider.hpp @@ -3,6 +3,8 @@ #include #include +#include +#include #include namespace data_access @@ -101,6 +103,33 @@ namespace data_access private: }; + struct unit_error_log_key { + std::string requester_name; + std::string requester_variable; + std::string provider_name; + std::string provider_variable; + std::string failure_message; + + bool operator<(unit_error_log_key const& rhs) const { + return std::tie(requester_name, requester_variable, provider_name, provider_variable, failure_message) + < std::tie(rhs.requester_name, rhs.requester_variable, rhs.provider_name, rhs.provider_variable, rhs.failure_message); + } + + bool operator==(unit_error_log_key const& rhs) const { + return std::tie(requester_name, requester_variable, provider_name, provider_variable, failure_message) + == std::tie(rhs.requester_name, rhs.requester_variable, rhs.provider_name, rhs.provider_variable, rhs.failure_message); + } + }; + + extern std::set unit_errors_reported; + + struct unit_conversion_exception : public std::runtime_error { + unit_conversion_exception(std::string message) : std::runtime_error(message) {} + std::string provider_model_name; + std::string provider_bmi_var_name; + std::string provider_units; + std::vector unconverted_values; + }; } diff --git a/include/realizations/catchment/Bmi_Module_Formulation.hpp b/include/realizations/catchment/Bmi_Module_Formulation.hpp index b8604adb20..d845523c0d 100644 --- a/include/realizations/catchment/Bmi_Module_Formulation.hpp +++ b/include/realizations/catchment/Bmi_Module_Formulation.hpp @@ -486,8 +486,6 @@ namespace realization { /** A configured mapping of BMI model variable names to standard names for use inside the framework. */ std::map bmi_var_names_map; bool model_initialized = false; - bool unitGetValueErrLogged = false; - bool unitGetValuesErrLogged = false; std::vector OPTIONAL_PARAMETERS = { BMI_REALIZATION_CFG_PARAM_OPT__USES_FORCINGS diff --git a/include/realizations/catchment/Bmi_Multi_Formulation.hpp b/include/realizations/catchment/Bmi_Multi_Formulation.hpp index 286c0eeb33..7b8090b57c 100644 --- a/include/realizations/catchment/Bmi_Multi_Formulation.hpp +++ b/include/realizations/catchment/Bmi_Multi_Formulation.hpp @@ -583,6 +583,15 @@ namespace realization { //TODO: After merge PR#405, try re-adding support for index return nested_module->get_value(selector); } + catch (data_access::unit_conversion_exception &uce) { + // We asked for it as a dimensionless quantity, "1", just above + static bool no_conversion_message_logged = false; + if (!no_conversion_message_logged) { + no_conversion_message_logged = true; + LOG("Emitting output variables from Bmi_Multi_Formulation without unit conversion - see NGWPC-7604", LogLevel::WARNING); + } + return uce.unconverted_values[0]; + } // If there was any problem with the cast and extraction of the value, throw runtime error catch (std::exception &e) { std::string throw_msg; throw_msg.assign("Multi BMI formulation can't use associated data provider as a nested module" diff --git a/src/forcing/NetCDFPerFeatureDataProvider.cpp b/src/forcing/NetCDFPerFeatureDataProvider.cpp index 6c5efccf9e..2a202a9247 100644 --- a/src/forcing/NetCDFPerFeatureDataProvider.cpp +++ b/src/forcing/NetCDFPerFeatureDataProvider.cpp @@ -419,14 +419,12 @@ double NetCDFPerFeatureDataProvider::get_value(const CatchmentAggrDataSelector& } catch (const std::runtime_error& e) { - //minor change to aid debugging (log_stream is an output log stream for messages from the underlying library, therefore logging to both EWTS and library logger) - netcdf_ss << "NetCDFPerFeatureDataProvider: get_converted_value Unit conversion unsuccessful - Returning unconverted value! (" << e.what() << ")" << std::endl; - log_stream << netcdf_ss.str(); - LOG(netcdf_ss.str(), LogLevel::WARNING); netcdf_ss.str(""); - netcdf_ss << "=== Exiting get_value function ===" << std::endl; - log_stream << netcdf_ss.str(); - LOG(netcdf_ss.str(), LogLevel::WARNING); netcdf_ss.str(""); - return rvalue; + data_access::unit_conversion_exception uce(e.what()); + uce.provider_model_name = "NetCDFPerFeatureDataProvider " + catchment_id; + uce.provider_bmi_var_name = selector.get_variable_name(); + uce.provider_units = native_units; + uce.unconverted_values.push_back(rvalue); + throw uce; } return rvalue; diff --git a/src/realizations/catchment/Bmi_Formulation.cpp b/src/realizations/catchment/Bmi_Formulation.cpp index 2048a47a79..ebb7bb32e2 100644 --- a/src/realizations/catchment/Bmi_Formulation.cpp +++ b/src/realizations/catchment/Bmi_Formulation.cpp @@ -18,3 +18,7 @@ namespace realization BMI_REALIZATION_CFG_PARAM_REQ__MODEL_TYPE, }; } + +namespace data_access { + std::set unit_errors_reported; +} diff --git a/src/realizations/catchment/Bmi_Module_Formulation.cpp b/src/realizations/catchment/Bmi_Module_Formulation.cpp index 335cafde19..883f82d679 100644 --- a/src/realizations/catchment/Bmi_Module_Formulation.cpp +++ b/src/realizations/catchment/Bmi_Module_Formulation.cpp @@ -25,8 +25,14 @@ namespace realization { if (timestep != (next_time_step_index - 1)) { throw std::invalid_argument("Only current time step valid when getting output for BMI C++ formulation"); } - std::string output_str; + static bool no_conversion_message_logged = false; + if (!no_conversion_message_logged) { + no_conversion_message_logged = true; + LOG("Emitting output variables from Bmi_Module_Formulation without unit conversion - see NGWPC-7604", LogLevel::WARNING); + } + + std::string output_str; for (const std::string& name : get_output_variable_names()) { output_str += (output_str.empty() ? "" : ",") + std::to_string(get_var_value_as_double(0, name)); } @@ -175,19 +181,12 @@ namespace realization { return values; } catch (const std::runtime_error& e) { - // Log at least one error - if (!unitGetValuesErrLogged) { - bmiform_ss << "BMI Module Formulation: get_values Unit conversion unsuccessful - Returning unconverted value! (" << e.what() << ")" << std::endl; - unitGetValuesErrLogged = true; - LOG(bmiform_ss.str(), LogLevel::WARNING); bmiform_ss.str(""); - } - else { - #ifndef UDUNITS_QUIET - bmiform_ss << "BMI Module Formulation: get_values Unit conversion unsuccessful - Returning unconverted value! (" << e.what() << ")" << std::endl; - LOG(bmiform_ss.str(), LogLevel::WARNING); bmiform_ss.str(""); - #endif - } - return values; + data_access::unit_conversion_exception uce(e.what()); + uce.provider_model_name = get_bmi_model()->get_model_name(); + uce.provider_bmi_var_name = bmi_var_name; + uce.provider_units = native_units; + uce.unconverted_values = std::move(values); + throw uce; } } //This is unlikely (impossible?) to throw since a pre-check on available names is done above. Assert instead? @@ -230,19 +229,12 @@ namespace realization { return UnitsHelper::get_converted_value(native_units, value, output_units); } catch (const std::runtime_error& e){ - // Log at least one error - if (!unitGetValueErrLogged) { - bmiform_ss << "BMI Module Formulation: get_value Unit conversion unsuccessful - Returning unconverted value! (" << e.what() << ")" << std::endl; - unitGetValueErrLogged = true; - LOG(bmiform_ss.str(), LogLevel::WARNING); bmiform_ss.str(""); - } - else { - #ifndef UDUNITS_QUIET - bmiform_ss << "BMI Module Formulation: get_value Unit conversion unsuccessful - Returning unconverted value! (" << e.what() << ")" << std::endl; - LOG(bmiform_ss.str(), LogLevel::WARNING); bmiform_ss.str(""); - #endif - } - return value; + data_access::unit_conversion_exception uce(e.what()); + uce.provider_model_name = get_bmi_model()->get_model_name(); + uce.provider_bmi_var_name = bmi_var_name; + uce.provider_units = native_units; + uce.unconverted_values.push_back(value); + throw uce; } } @@ -724,10 +716,28 @@ namespace realization { value_ptr = get_values_as_type( type, values.begin(), values.end() ); } else { - //scalar value - double value = provider->get_value(CatchmentAggrDataSelector(this->get_catchment_id(),var_map_alias, model_epoch_time, t_delta, - get_bmi_model()->GetVarUnits(var_name))); - value_ptr = get_value_as_type(type, value); + try { + //scalar value + double value = provider->get_value(CatchmentAggrDataSelector(this->get_catchment_id(),var_map_alias, model_epoch_time, t_delta, + get_bmi_model()->GetVarUnits(var_name))); + value_ptr = get_value_as_type(type, value); + } catch (data_access::unit_conversion_exception &uce) { + data_access::unit_error_log_key key{get_id(), var_map_alias, uce.provider_model_name, uce.provider_bmi_var_name, uce.what()}; + auto ret = data_access::unit_errors_reported.insert(key); + bool new_error = ret.second; + if (new_error) { + std::stringstream ss; + ss << "Unit conversion failure:" + << " requester {'" << get_bmi_model()->get_model_name() << "' catchment '" << get_catchment_id() + << "' variable '" << var_name << "'" << " (alias '" << var_map_alias << "')" + << " units '" << get_bmi_model()->GetVarUnits(var_name) << "'}" + << " provider {'" << uce.provider_model_name << "' source variable '" << uce.provider_bmi_var_name << "'" + << " raw value " << uce.unconverted_values[0] << "}" + << " message \"" << uce.what() << "\""; + LOG(ss.str(), LogLevel::WARNING); ss.str(""); + } + value_ptr = get_value_as_type(type, uce.unconverted_values[0]); + } } get_bmi_model()->SetValue(var_name, value_ptr.get()); } diff --git a/test/bmi/Bmi_Cpp_Adapter_Test.cpp b/test/bmi/Bmi_Cpp_Adapter_Test.cpp index 0151360852..d9e7ad6d2d 100644 --- a/test/bmi/Bmi_Cpp_Adapter_Test.cpp +++ b/test/bmi/Bmi_Cpp_Adapter_Test.cpp @@ -51,7 +51,7 @@ class Bmi_Cpp_Adapter_Test : public ::testing::Test { std::vector expected_output_var_names = { "OUTPUT_VAR_1", "OUTPUT_VAR_2" }; std::vector expected_output_var_locations = { "node", "node" }; std::vector expected_output_var_grids = { 0, 0 }; - std::vector expected_output_var_units = { "m", "m" }; + std::vector expected_output_var_units = { "mm/s", "m" }; std::vector expected_output_var_types = { "double", "double" }; int expected_grid_rank = 1; int expected_grid_size = 1; diff --git a/test/bmi/Bmi_Fortran_Adapter_Test.cpp b/test/bmi/Bmi_Fortran_Adapter_Test.cpp index 1e46792dc6..1e3982dfbe 100644 --- a/test/bmi/Bmi_Fortran_Adapter_Test.cpp +++ b/test/bmi/Bmi_Fortran_Adapter_Test.cpp @@ -52,7 +52,7 @@ class Bmi_Fortran_Adapter_Test : public ::testing::Test { std::vector expected_output_var_locations = { "node", "node", "node", "node", "node"}; std::vector expected_output_var_grids = { 0, 0, 0, 1, 2, 2 }; std::vector expected_input_var_grids = { 0, 0, 0, 1 }; - std::vector expected_output_var_units = { "m", "m", "s", "m", "m", "m" }; + std::vector expected_output_var_units = { "mm s^-1", "m", "s", "m", "m", "m" }; std::vector expected_output_var_types = { "double precision", "real", "integer", "double precision", "double precision", "double precision" }; std::vector expected_output_var_item_sizes = { 8, 4, 4, 8, 8, 8}; std::vector expected_input_var_types = { "double precision", "real", "integer", "double precision" }; diff --git a/test/forcing/CsvPerFeatureForcingProvider_Test.cpp b/test/forcing/CsvPerFeatureForcingProvider_Test.cpp index 9d292f1f08..774b3e7633 100644 --- a/test/forcing/CsvPerFeatureForcingProvider_Test.cpp +++ b/test/forcing/CsvPerFeatureForcingProvider_Test.cpp @@ -94,24 +94,24 @@ TEST_F(CsvPerFeatureForcingProviderTest, TestForcingDataRead) time_t t = begin+(i*3600); std::cerr << std::ctime(&t) << std::endl; - current_precipitation = Forcing_Object->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_LIQUID_EQ_PRECIP_RATE, begin+(i*3600), 3600, ""), data_access::SUM); + current_precipitation = Forcing_Object->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_LIQUID_EQ_PRECIP_RATE, begin+(i*3600), 3600, "mm/s"), data_access::SUM); EXPECT_NEAR(current_precipitation, 7.9999999999999996e-07, 0.00000005); - double temp_k = Forcing_Object->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_SURFACE_TEMP, begin+(i*3600), 3600, ""), data_access::MEAN); + double temp_k = Forcing_Object->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_SURFACE_TEMP, begin+(i*3600), 3600, "K"), data_access::MEAN); EXPECT_NEAR(temp_k, 286.9, 0.00001); int current_epoch; i = 387; - current_precipitation = Forcing_Object->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_LIQUID_EQ_PRECIP_RATE, begin+(i*3600), 3600, ""), data_access::SUM); + current_precipitation = Forcing_Object->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_LIQUID_EQ_PRECIP_RATE, begin+(i*3600), 3600, "mm/s"), data_access::SUM); EXPECT_NEAR(current_precipitation, 6.9999999999999996e-07, 0.00000005); //Check exceeding the forcing range to retrieve the last forcing precipation rate i = 388; - current_precipitation = Forcing_Object->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_LIQUID_EQ_PRECIP_RATE, begin+(i*3600), 3600, ""), data_access::SUM); + current_precipitation = Forcing_Object->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_LIQUID_EQ_PRECIP_RATE, begin+(i*3600), 3600, "mm/s"), data_access::SUM); EXPECT_NEAR(current_precipitation, 6.9999999999999996e-07, 0.00000005); } @@ -127,18 +127,18 @@ TEST_F(CsvPerFeatureForcingProviderTest, TestForcingDataReadAltFormat) time_t t = begin+(i*3600); std::cerr << std::ctime(&t) << std::endl; - current_precipitation = Forcing_Object_2->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_LIQUID_EQ_PRECIP_RATE, begin+(i*3600), 3600, ""), data_access::SUM); + current_precipitation = Forcing_Object_2->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_LIQUID_EQ_PRECIP_RATE, begin+(i*3600), 3600, "mm/s"), data_access::SUM); EXPECT_NEAR(current_precipitation, 0.00032685, 0.00000001); - double temp_k = Forcing_Object_2->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_SURFACE_TEMP, begin+(i*3600), 3600, ""), data_access::MEAN); + double temp_k = Forcing_Object_2->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_SURFACE_TEMP, begin+(i*3600), 3600, "K"), data_access::MEAN); EXPECT_NEAR(temp_k, 265.77, 0.00001); int current_epoch; i = 34; - current_precipitation = Forcing_Object_2->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_LIQUID_EQ_PRECIP_RATE, begin+(i*3600), 3600, ""), data_access::SUM); + current_precipitation = Forcing_Object_2->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_LIQUID_EQ_PRECIP_RATE, begin+(i*3600), 3600, "mm/s"), data_access::SUM); EXPECT_NEAR(current_precipitation, 0.00013539, 0.00000001); @@ -156,7 +156,7 @@ TEST_F(CsvPerFeatureForcingProviderTest, TestForcingDataUnitConversion) time_t t = begin+(i*3600); std::cerr << std::ctime(&t) << std::endl; - current_precipitation = Forcing_Object->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_LIQUID_EQ_PRECIP_RATE, begin+(i*3600), 3600, ""), data_access::SUM); + current_precipitation = Forcing_Object->get_value(CatchmentAggrDataSelector("", CSDMS_STD_NAME_LIQUID_EQ_PRECIP_RATE, begin+(i*3600), 3600, "mm/s"), data_access::SUM); EXPECT_NEAR(current_precipitation, 7.9999999999999996e-07, 0.00000005); @@ -188,9 +188,14 @@ TEST_F(CsvPerFeatureForcingProviderTest, TestForcingUnitHeaderParsing) {"U2D", "m s-1", "cm s-1"}, {"V2D", "m/s", "cm s-1"}, {"TEST", "kg", "g"}, + // Disable the cases where units won't be parsed and + // conversion was previously not expected. The non-conversion + // is now being treated as an error, per NGWPC-7604 +#if 0 {"PSFC[Pa)", "Pa", "bar"}, {"SWDOWN(W m-2]", "W m-2", "langley"}, {"LWDOWN [alt]", "W m-2", "langley"} +#endif }; for (auto ite = expected.begin(); ite != expected.end(); ite++) {