Skip to content
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.

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

When used via BMI, precip should NOT be scaled any further #128

Open
hellkite500 opened this issue Sep 2, 2024 · 4 comments
Open

When used via BMI, precip should NOT be scaled any further #128

hellkite500 opened this issue Sep 2, 2024 · 4 comments

Comments

@hellkite500
Copy link
Member

https://github.com/NOAA-OWP/cfe/blob/0b488a123887100c6304aee3b0149bc97a68283e/src/bmi_cfe.c#L1509C7-L1509C81

During the update step, precip is scaled by 1000 assuming the prcipitation input variable has units of mm/hr which is NOT a valid assumption when precip is coming from a BMI input variable. For example, when copuled with NOM and passing QINSUR as the surface flux, then the forcing comes in the units of m/s.

Also, when used via a framework which uses BMI unit data to perform unit conversion, this becomes prolematic, as the advertised unit of the precip input variable is mm/hr and so, as in the example with NOM, a converting framework will pass in values with the correct units, and they are then divided by 1000.

@andywood
Copy link

andywood commented Sep 2, 2024

I agree, hardwired input/output unit conversions in formulations for NextGen are to be avoided.

@SnowHydrology
Copy link
Contributor

I believe this is legacy code from the standalone CFE running with AORC forcing that it reads itself. If we update the code to remove the hard-coded transformation (which we should) we'll also need to update the standalone forcing data to reflect this. Or, account for the change in some other way.

@PhilMiller
Copy link
Contributor

One easy way would be to just change what unit the BMI input variable advertises, to match the scaling. Everything else should just do the right thing then, I think?

In an ideal world, we would not have hard-coded scaling and conversion, of course, but changing the rest of the world around the code may not be the most expedient path.

@ajkhattak
Copy link
Contributor

this should be a pretty straightforward change , unless I am missing something.
so we should change the unit mm h-1 to m h-1 (

"mm h-1", //"atmosphere_water__liquid_equivalent_precipitation_rate"
)
and remove the 1000. from denominator.

For the standalone runs, we can do the conversion from mm to m inside the main.c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants