Inconsistencies in variable units and BMI implementation

[robertbartel]

There are several inconsistencies related to the units for some variables in the CFE implementation, which could potentially cause problems when interacting with the module via BMI. While there might be others as well, (e.g., DEEP_GW_TO_CHANNEL_FLUX), the most apparent is with these BMI variables.

BMI Variable Name	Type	Units
RAIN_RATE	output	m
atmosphere_water__liquid_equivalent_precipitation_rate	input	kg m-2

The listed units are those returned by the CFE implementation of the BMI Get_var_units function.

The problem

1. there are reasons to think these units aren't the intended/actual units of these variables
2. it turns out that RAIN_RATE and atmosphere_water__liquid_equivalent_precipitation_rate are literally the same variable in memory, but with different advertised units (see item 1.)

Despite the names, these variables aren't rates based on their units, at least not with respect to time. This seems likely to be incorrect for atmosphere_water__liquid_equivalent_precipitation_rate, and almost certainly is for RAIN_RATE (units of just m don't make sense for a rate). Some usage of the variables also is suggestive of a time component.

If either of these variables do have the correct units advertised, then some additional documentation clarifying the apparent terminology inconsistency would help with confusion.

It's impossible, however, for both variables to have the correct units advertised. The BMI Get_value_ptr function will return &cfe_ptr->aorc.precip_kg_per_m2 as the pointer for either the RAIN_RATE variable or the atmosphere_water__liquid_equivalent_precipitation_rate variable. I.e., both BMI variables are using the same memory address, so they are in fact the same thing. This behavior by itself is valid, but it doesn't make sense with the current unit metadata.

[jmframe]

The Get_value_ptr for RAIN_RATE is not implemented correctly, it should return cfe_ptr->timestep_rainfall_input_m, rather than cfe_ptr->aorc.precip_kg_per_m2. Although I am not sure why CFE should have RAIN_RATE in the output variables...

[robertbartel]

@jmframe ah, interesting. Doing a quick search, I found this section where that other variable is being set:

// Two modes to get forcing data... 0/FALSE) read from file, 1/TRUE) pass with bmi    
    if (cfe_ptr->is_forcing_from_bmi == TRUE)
      // BMI sets the precipitation to the aorc structure.
      // divide by 1000 to convert from mm/h to m w/ 1h timestep as per t-shirt_0.99f
      cfe_ptr->timestep_rainfall_input_m = cfe_ptr->aorc.precip_kg_per_m2 /1000;
    else
      // Set the current rainfall input to the right place in the forcing array.
      // divide by 1000 to convert from mm/h to m w/ 1h timestep as per t-shirt_0.99f
      cfe_ptr->timestep_rainfall_input_m = cfe_ptr->forcing_data_precip_kg_per_m2[cfe_ptr->current_time_step] /1000;

So even after Get_value_ptr is corrected in how it handles RAIN_RATE, there will still be essentially the same issue with the relationship versus the units for these two values.

[SnowHydrology]

Is there a need for RAIN_RATE to be listed as a BMI output var? If not, it can be removed. Additionally, please note the change in the BMI specification so that precipitation is now given as mm h^-1:

cfe/src/bmi_cfe.c

Line 222 in e8c2d4b

"mm h-1", //"atmosphere_water__liquid_equivalent_precipitation_rate"

[stcui007]

Yes. It is helpful for comparing how the various calculated output flows respond to the original input.

…

On Thu, Nov 18, 2021 at 9:23 AM K. Jennings ***@***.***> wrote: Is there a need for RAIN_RATE to be listed as a BMI output var? If not, it can be removed. Additionally, please note the change in the BMI specification so that precipitation is now given as mm h-1: https://github.com/NOAA-OWP/cfe/blob/e8c2d4b0391cec7d4191beb16f8c10650e3e02e2/src/bmi_cfe.c#L222 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#29 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACA4SRI6CZ6MGIYLH76MBULUMULATANCNFSM5IF5DW2Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[SnowHydrology]

@stcui007 that makes sense. Is there a reason you can't use atmosphere_water__liquid_equivalent_precipitation_rate or does the model engine only expose the output variables for writing simulation data?

[stcui007]

@keith Jennings - NOAA Affiliate ***@***.***> In the config file, there is this line: "atmosphere_water__liquid_equivalent_precipitation_rate": "precip_rate", I assume RAIN_RATE is equivalent to precip_rate? Last time when RAIN_RATE was removed, it caused a runtime error in running ngen and Jonathan had to put it back. Perhaps we don't want to spend too much time on it for now? Anyway, we need RAIN_RATE or its equivalent in the same output file for comparison and easy post processing.

…

On Thu, Nov 18, 2021 at 11:48 AM K. Jennings ***@***.***> wrote: @stcui007 <https://github.com/stcui007> that makes sense. Is there a reason you can't use atmosphere_water__liquid_equivalent_precipitation_rate or does the model engine only expose the output variables for writing simulation data? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACA4SRKTADF7BKTJMIQA7SDUMU36TANCNFSM5IF5DW2Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.