Fix 384, prioritize use of BMI potential evapotranspiration

[hellkite500]

The current implementation uses internal PET kernels to calc_et() in the framework when a module request an evapotranspiration variable as input. This is done regardless of whether or not that variable is available from another BMI module source. This PR is backwards compatible, and will still provide PET when no BMI module in the stack knows how to produce the PET standard name, but it allows for a BMI module to be used first if it is capable of producing the value.

Closes #384.

Additions

• protected check_internal_providers function to look for forcing variables that aren't available from BMI providers.

Removals

Changes

• Implement TODO function for finding bmi_var_name as function get_bmi_var_name
• If a BMI var name isn't found in output variables or name map, check for internal providers of that data

Testing

1. Passes all existing unit tests

Checklist

• PR has an informative and human-readable title
• x] Changes are limited to a single goal (no scope creep)
• Code can be automatically merged (no conflicts)
• Code follows project standards (link if applicable)
• Passes all existing automated tests
• Any change in functionality is tested
• New functions are documented (with a description, list of inputs, and expected output)
• Placeholder code is flagged / future todos are captured in comments
• Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
• Reviewers requested with the Reviewers tool ➡️

Target Environment support

• Linux
• MacOS

[hellkite500]

Looking at the one test failure, here are the relevant 3 failures:

[ RUN      ] Bmi_C_Formulation_Test.GetOutputLineForTimestep_0_a
/Users/runner/work/ngen/ngen/test/realizations/catchments/Bmi_C_Formulation_Test.cpp:308: Failure
Expected equality of these values:
  output
    Which is: "0.000000,-0.000000"
  "0.000000,0.000000"
Unit conversion error: 
[  FAILED  ] Bmi_C_Formulation_Test.GetOutputLineForTimestep_0_a (80 ms)
Unable to convert mm s^-1 to m
[ RUN      ] Bmi_C_Formulation_Test.GetOutputLineForTimestep_1_a
Returning unconverted value!
/Users/runner/work/ngen/ngen/test/realizations/catchments/Bmi_C_Formulation_Test.cpp:324: Failure
Expected equality of these values:
  output
    Which is: "-0.000000,0.000000"
  "0.000000,0.000000"
[  FAILED  ] Bmi_C_Formulation_Test.GetOutputLineForTimestep_1_a (114 ms)
Unit conversion error: 
[ RUN      ] Bmi_C_Formulation_Test.GetOutputLineForTimestep_1_b
Unable to convert mm s^-1 to m
Returning unconverted value!
Unit conversion error: 
Unable to convert mm s^-1 to m
Returning unconverted value!
Unit conversion error: 
Unable to convert mm s^-1 to m
/Users/runner/work/ngen/ngen/test/realizations/catchments/Bmi_C_Formulation_Test.cpp:339: Failure
Returning unconverted value!
Expected equality of these values:
Unit conversion error: 
  output
Unable to convert mm s^-1 to m
    Which is: "-0.000000,0.000002"
Returning unconverted value!
  "0.000000,0.000002"
Unit conversion error: 
[  FAILED  ] Bmi_C_Formulation_Test.GetOutputLineForTimestep_1_b (133 ms)

Not entirely sure why at the moment these values are coming through as -0.000000.

[hellkite500]

4ccd04c might be relevant here...

[robertbartel]

Heh, I remember this working differently.

@hellkite500, I'll wait on final approval, since I expect more changes are going to be needed to address the problem with the checks, but otherwise this looks good.

[hellkite500]

I think the failing test may be fixed by d230f8b

[robertbartel]

@hellkite500, that check still fails. Is d230f8b actually included in your branch? I did a quick search of the log and didn't see it. Ah, never mind. I can't find the commit, but I do see the changes. And they work for me locally. Very curious ...

[hellkite500]

I did a quick search of the log and didn't see it.~ Ah, never mind. I can't find the commit, but I do see the changes. And they work for me locally. Very curious ...

That commit was merged into master with #389...

Not sure why I cannot get the strings to match now...

[mattw-nws]

Might suggest EXPECT_THAT with MatchesRegex, might give better output on the test failure.

[hellkite500]

Might suggest EXPECT_THAT with MatchesRegex, might give better output on the test failure.

Found googlemock, trying to figure out if it is built in our configuration and where it puts the required headers to use it...

[hellkite500]

Went from a c++ std::regex check to a google mock EXPECT_THAT(output, MatchesRegex(...)) and now the runner works??? Still a bit of a mystery why it failed the original regex check 🤔 .

[mattw-nws]

Place your bets now on whether it fails again in the post-merge checks?

One small question...you only checked 5 of the six zero values for negativity...was that deliberate?

Never mind... that's not even zero, sorry. And... now it's 0.000001 where it was 0.000002... which I guess is because of #389... not confusing at all.

[hellkite500]

So a non-deterministic error seems to exist relating to this PR. In the post-merge check:

/Users/runner/work/ngen/ngen/test/realizations/catchments/Bmi_C_Formulation_Test.cpp:310: Failure
Value of: output
Expected: matches regular expression "-?0.000000,-?0.000000"
  Actual: "0.000000,-4.000000"
[  FAILED  ] Bmi_C_Formulation_Test.GetOutputLineForTimestep_0_a (65 ms)
[ RUN      ] Bmi_C_Formulation_Test.GetOutputLineForTimestep_1_a
/Users/runner/work/ngen/ngen/test/realizations/catchments/Bmi_C_Formulation_Test.cpp:326: Failure
Value of: output
Expected: matches regular expression "-?0.000000,-?0.000000"
  Actual: "-4.000000,0.000000"

I'll open an issue to track this...

[mattw-nws]

Place your bets now on whether it fails again in the post-merge checks?

Called it, apparently :-\