Skip to content

unit conversion error changes #52

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cmaynard-ngwpc merged 1 commit into from
Sep 29, 2025
Merged

unit conversion error changes #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/realizations/catchment/Bmi_Multi_Formulation.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,7 +579,7 @@ namespace realization {
try {
auto const& nested_module = data_provider_iter->second;
long nested_module_time = nested_module->get_data_start_time() + ( this->get_model_current_time() - this->get_model_start_time() );
auto selector = CatchmentAggrDataSelector(this->get_catchment_id(),var_name,nested_module_time,this->record_duration(),"1");
auto selector = CatchmentAggrDataSelector(this->get_catchment_id(),var_name,nested_module_time,this->record_duration(),"");
Copy link

@PhilMiller PhilMiller Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace got messed up here. This was fixed in #40, but this branch apparently was a cherry-pick rather than a rebase?

//TODO: After merge PR#405, try re-adding support for index
return nested_module->get_value(selector);
}
Expand Down
32 changes: 25 additions & 7 deletions src/core/mediator/UnitsHelper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,21 +71,39 @@ double UnitsHelper::get_converted_value(const std::string &in_units, const doubl

double* UnitsHelper::convert_values(const std::string &in_units, double* in_values, const std::string &out_units, double* out_values, const size_t& count)
{
if(in_units == out_units){
// Early-out optimization
if(in_values == out_values){
auto is_noneish = [](const std::string& u)->bool {
return u.empty() || u == "none" || u == "unitless" || u == "dimensionless" || u == "-";
};

// Normalize input units: map none-ish → "1"
const std::string in_norm = is_noneish(in_units) ? std::string("1") : in_units;

// Normalize requested units:
// - none-ish → "1" if input is "1"; otherwise "" (unspecified → skip conversion)
std::string out_norm;
if (is_noneish(out_units)) {
out_norm = (in_norm == "1") ? std::string("1") : std::string("");
}
else {
out_norm = out_units;
}

// Early outs (no UDUNITS parsing or converter creation)
if (out_norm.empty() || in_norm == out_norm) {
// Pass-through
if (in_values == out_values) {
return in_values;
} else {
memcpy(out_values, in_values, sizeof(double)*count);
std::memcpy(out_values, in_values, sizeof(double)*count);
return out_values;
}
}

std::call_once(unit_system_inited, init_unit_system);
auto converter = get_converter(in_units, out_units);

auto converter = get_converter(in_norm, out_norm);

cv_convert_doubles(converter.get(), in_values, count, out_values);

return out_values;
}

42 changes: 32 additions & 10 deletions src/realizations/catchment/Bmi_Module_Formulation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ namespace realization {
throw std::runtime_error("Bmi_Singular_Formulation does not yet implement get_ts_index_for_time");
}

std::vector<double> Bmi_Module_Formulation::get_values(const CatchmentAggrDataSelector& selector, data_access::ReSampleMethod m)
std::vector<double> Bmi_Module_Formulation::get_values(const CatchmentAggrDataSelector& selector, data_access::ReSampleMethod m)
{
std::string output_name = selector.get_variable_name();
time_t init_time = selector.get_init_time();
Expand Down Expand Up @@ -176,15 +176,24 @@ namespace realization {

// Convert units
std::string native_units = get_bmi_model()->GetVarUnits(bmi_var_name);

// --- minimal addition: normalize "none"/"" to "1" and skip conversion if equal ---
std::string in_units_norm = (native_units.empty() || native_units == "none") ? "1" : native_units;
std::string out_units_norm = (output_units.empty() || output_units == "none") ? "1" : output_units;
if (in_units_norm == out_units_norm) {
return values;
}
// -------------------------------------------------------------------------------

try {
UnitsHelper::convert_values(native_units, values.data(), output_units, values.data(), values.size());
UnitsHelper::convert_values(in_units_norm, values.data(), out_units_norm, values.data(), values.size());
return values;
}
catch (const std::runtime_error& e) {
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.provider_units = native_units; // keep original for diagnostics
uce.unconverted_values = std::move(values);
throw uce;
}
Expand Down Expand Up @@ -225,14 +234,23 @@ namespace realization {

// Convert units
std::string native_units = get_bmi_model()->GetVarUnits(bmi_var_name);

// --- minimal addition: normalize "none"/"" to "1" and skip conversion if equal ---
std::string in_units_norm = (native_units.empty() || native_units == "none") ? "1" : native_units;
std::string out_units_norm = (output_units.empty() || output_units == "none") ? "1" : output_units;
if (in_units_norm == out_units_norm) {
return value;
}
// -------------------------------------------------------------------------------

try {
return UnitsHelper::get_converted_value(native_units, value, output_units);
return UnitsHelper::get_converted_value(in_units_norm, value, out_units_norm);
}
catch (const std::runtime_error& e){
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.provider_units = native_units; // keep original for diagnostics
uce.unconverted_values.push_back(value);
throw uce;
}
Expand All @@ -242,7 +260,6 @@ namespace realization {
throw std::runtime_error(get_formulation_type() + " received invalid output forcing name " + output_name);
}


static bool is_var_name_in_collection(const std::vector<std::string> &all_names, const std::string &var_name) {
return std::count(all_names.begin(), all_names.end(), var_name) > 0;
}
Expand Down Expand Up @@ -663,7 +680,7 @@ namespace realization {
"': no logic for converting value to variable's type.");
}

void Bmi_Module_Formulation::set_model_inputs_prior_to_update(const double &model_init_time, time_step_t t_delta) {
void Bmi_Module_Formulation::set_model_inputs_prior_to_update(const double &model_init_time, time_step_t t_delta) {
std::vector<std::string> in_var_names = get_bmi_model()->GetInputVarNames();
time_t model_epoch_time = convert_model_time(model_init_time) + get_bmi_model_start_time_forcing_offset_s();

Expand Down Expand Up @@ -692,10 +709,16 @@ namespace realization {
// Finally, use the value obtained to set the model input
std::string type = get_bmi_model()->get_analogous_cxx_type(get_bmi_model()->GetVarType(var_name),
varItemSize);

// Minimal change: normalize requested units (treat ""/none as dimensionless "1")
std::string consumer_units = get_bmi_model()->GetVarUnits(var_name);
if (consumer_units.empty() || consumer_units == "none")
consumer_units = "1";

if (numItems != 1) {
//more than a single value needed for var_name
auto values = provider->get_values(CatchmentAggrDataSelector(this->get_catchment_id(),var_map_alias, model_epoch_time, t_delta,
get_bmi_model()->GetVarUnits(var_name)));
consumer_units));
//need to marshal data types to the receiver as well
//this could be done a little more elegantly if the provider interface were
//"type aware", but for now, this will do (but requires yet another copy)
Expand All @@ -719,7 +742,7 @@ namespace realization {
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)));
consumer_units));
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()};
Expand All @@ -743,7 +766,6 @@ namespace realization {
}
}


void Bmi_Module_Formulation::append_model_inputs_to_stream(const double &model_init_time, time_step_t t_delta, std::stringstream &inputs) {
std::vector<std::string> in_var_names = get_bmi_model()->GetInputVarNames();
time_t model_epoch_time = convert_model_time(model_init_time) + get_bmi_model_start_time_forcing_offset_s();
Expand Down
Loading