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.

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

Jump to bottom

Fix for #453 #460

Merged
merged 11 commits into from
Nov 3, 2022
Merged

Fix for #453 #460

merged 11 commits into from
Nov 3, 2022

Conversation

stcui007
Copy link
Contributor

@stcui007 stcui007 commented Oct 24, 2022 •
edited
Loading

[Short description explaining the high-level reason for the pull request]

Additions

data/example_realization_config_w_bmi_c__lin_mac.json
data/bmi/c/pet/cat-52_bmi_config.ini

Removals

Changes

data/example_bmi_multi_realization_config.json
data/example_bmi_multi_realization_config_w_noah_pet_cfe.json
data/bmi/fortran/noah-owp-modular-init-cat-27.namelist.input
data/bmi/fortran/noah-owp-modular-init-cat-52.namelist.input
data/bmi/fortran/noah-owp-modular-init-cat-67.namelist.input

Testing

Ran ngen tests on Linux machines.

Screenshots

Notes

Todos

Checklist

  • PR has an informative and human-readable title
  • 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 ➡️

Testing checklist (automated report can be put here)

Target Environment support

  • [x ] Linux

@mattw-nws
Copy link
Contributor

Am I correct in understanding that this is supposed to address #453 ?

mattw-nws
mattw-nws requested changes Oct 24, 2022
View reviewed changes
data/example_bmi_multi_realization_config.json Outdated
@@ -59,7 +59,8 @@
"main_output_variable": "Q_OUT",
"registration_function": "register_bmi_cfe",
"variables_names_map": {
"water_potential_evaporation_flux": "ETRAN",
"water_potential_evaporation_flux": "water_potential_evaporation_flux",
Copy link
Contributor

Choose a reason for hiding this comment

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

This bypasses Noah's ETRAN variable and uses the old in-framework ET calculator... I don't think we want to do this. Was that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using ETRAB somehow generate a runtime error if I remember correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some re-testing, it seems to work ok with ETRAN in place. The runtime error might be caused by something else. So we can reverse back.

data/example_bmi_multi_realization_config.json Outdated
@@ -59,7 +59,8 @@
"main_output_variable": "Q_OUT",
"registration_function": "register_bmi_cfe",
"variables_names_map": {
"water_potential_evaporation_flux": "ETRAN",
"water_potential_evaporation_flux": "water_potential_evaporation_flux",
"atmosphere_water__liquid_equivalent_precipitation_rate" : "APCP_surface",
Copy link
Contributor

Choose a reason for hiding this comment

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

This bypasses Noah's QINSUR variable.... though I suppose this was happening by default before anyway. This should probably be QINSUR instead of APCP_surface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the reply to the last comment.

test/core/nexus/NexusTests.cpp Outdated
@@ -47,7 +47,7 @@ TEST_F(Nexus_Test, TestInit0)
//HY_HydroLocationType type(HY_HydroLocationType::undefined); //!< Test data type
//HY_IndirectPosition pos;
//std::shared_ptr<HY_HydroLocation>location = std::make_shared<HY_HydroLocation>(point, type, pos);
std::vector<string> contrib = {"cat-1"};
std::vector<std::string> contrib = {"cat-1"};
Copy link
Contributor

Choose a reason for hiding this comment

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

This is out of scope, probably revert it, I have a branch in work right now to correct several compiler issues... it should pass tests without this (so far), so just leave the change in your working copy if you need it and don't commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I left it as is so it can pass the runner tests. If it can pass the tests without it, I would be happy to leave it out.

data/bmi/fortran/noah-owp-modular-init-cat-27.namelist.input Outdated
@@ -1,32 +1,35 @@
&timing ! and output
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes require a version of Noah that hasn't been pinned yet in ngen... so the tests are failing. Revert all these but save them, we should probably make a separate PR to update Noah... and that plus these edits will have to be done in the same PR to make the tests pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been wondering why the tests are failing? I used the most recent Noah and everything seems to be working locally. There are apparently incorrect variable names in it such as "input_filename" which should be "forcing_filename".

data/bmi/fortran/noah-owp-modular-init-cat-52.namelist.input Outdated
@@ -1,32 +1,35 @@
&timing ! and output
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes require a version of Noah that hasn't been pinned yet in ngen... so the tests are failing. Revert all these but save them, we should probably make a separate PR to update Noah... and that plus these edits will have to be done in the same PR to make the tests pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the reply to the previous comment.

data/bmi/fortran/noah-owp-modular-init-cat-67.namelist.input Outdated
@@ -1,32 +1,35 @@
&timing ! and output
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes require a version of Noah that hasn't been pinned yet in ngen... so the tests are failing. Revert all these but save them, we should probably make a separate PR to update Noah... and that plus these edits will have to be done in the same PR to make the tests pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the reply to the previous comment.

data/bmi/fortran/parameters/GENPARM.TBL Outdated
@@ -0,0 +1,38 @@
General Parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes require a version of Noah that hasn't been pinned yet in ngen... so the tests are failing. Revert all these but save them, we should probably make a separate PR to update Noah... and that plus these edits will have to be done in the same PR to make the tests pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the reply to the previous comment.

data/bmi/fortran/parameters/MPTABLE.TBL Outdated
@@ -0,0 +1,650 @@
&usgs_veg_categories
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes require a version of Noah that hasn't been pinned yet in ngen... so the tests are failing. Revert all these but save them, we should probably make a separate PR to update Noah... and that plus these edits will have to be done in the same PR to make the tests pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the reply to the previous comment.

data/bmi/fortran/parameters/SOILPARM.TBL Outdated
@@ -0,0 +1,45 @@
Soil Parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes require a version of Noah that hasn't been pinned yet in ngen... so the tests are failing. Revert all these but save them, we should probably make a separate PR to update Noah... and that plus these edits will have to be done in the same PR to make the tests pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the reply to the previous comment.

data/example_realization_config_unified.json Outdated
@@ -0,0 +1,230 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not understand what this file is. Is it supposed to be a merger of example_bmi_multi_realization_config.json and example_bmi_multi_realization_config__macos.json? I don't understand why SLoTH and SFT and SMP are now in here. And I don't think this realization will even work with the versions of Noah and CFE that are currently pinned in /extern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is modeled after example_realization_config_w_bmi_c__linux.json and the MACOS version and combined them. We already have a updated example_bmi_multi_realization_config.json that is working with Noah, at least locally. I thought to add the SLoTH and SFT and SMP to include more modules so more up to date. The SLoTH and SFT and SMP part is basically taken from config in Soil Moisture modules. This one does not include Noah. The Noah is included in example_bmi_multi_realization_config.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file also contains some older models that are still in Ngen. Perhaps it is still of use?

@stcui007
Copy link
Contributor Author

stcui007 commented Oct 24, 2022 via email

@mattw-nws
Copy link
Contributor

mattw-nws commented Oct 26, 2022 •
edited
Loading

I see what you meant about renaming example_bmi_multi_realization_config.json ... okay, so now the filename is not inconsistent with what's contained in the file--good. But what was described in #453 was to make sure there was still a bmi_c -only example. We can perhaps keep this bmi_multi example (though I don't know that it adds much value since other similar examples already exist). But, please add a bmi-c-only example with a name similar to the old file that uses the module in extern/test_bmi_c.

@stcui007
Copy link
Contributor Author

The test_bmi_c part is in data/example_realization_config_w_bmi_c__lin_mac.json

@mattw-nws
Copy link
Contributor

So, three things:

  1. As mentioned above, we are trying to get a bmi-c-ONLY example. That is, this file and this line should not contain bmi_multi when its filename says bmi_c... and we do want the filename to be bmi_c.
  2. There is no reason for data/example_bmi_multi_realization_config.json to include libtestbmicmodel at all... we are only messing with the test_bmi_c module to achieve Add initial project reference material to doc #1 above, and it adds nothing to have it in the bmi_multi examples.
  3. The tests on the previous commit failed because there was no build step for libtestbmicmodel.so ... that said, given the name of the test, it probably makes more sense to not fix that problem, and instead just remove test_bmi_c from the realization config file as mentioned in Sample data for ngen. #2. After this, the test will probably succeed. However, we should make sure that the example_realization_config_w_bmi_c__lin_mac.json realization does actually work, at least by manual test.

@stcui007
Copy link
Contributor Author

stcui007 commented Nov 1, 2022

The config json file needs more verification.

@mattw-nws
Copy link
Contributor

mattw-nws commented Nov 1, 2022 •
edited
Loading

Okay this is almost ready now I think... There are a few follow-on items not in the original description that would be good:

  1. Can we add to this the deletion of example_realization_config_w_bmi_c__linux.json and example_realization_config_w_bmi_c__mac.json (since the new lin_mac file replaces them)...
  2. Replace the filename example_realization_config_w_bmi_c__linux.json in Bmi_Testing_Util.hpp with data/example_realization_config_w_bmi_c__lin_mac.json (this part of the utility does not appear to be used anywhere so this should be all that's needed)
  3. Edit the text at the bottom of REALIZATION_CONFIG.md to point to the single example_realization_config_w_bmi_c__lin_mac.json file.

@stcui007
Copy link
Contributor Author

stcui007 commented Nov 1, 2022

Working on them.

@stcui007
Copy link
Contributor Author

stcui007 commented Nov 1, 2022 via email

@mattw-nws mattw-nws linked an issue Nov 3, 2022 that may be closed by this pull request
Example realization file tune-up/catch-up after #448 and #430 #453
Closed
@mattw-nws mattw-nws changed the title Setting up some config json files Fix for #453 Nov 3, 2022
@mattw-nws mattw-nws merged commit e5cf1b7 into NOAA-OWP:master Nov 3, 2022
@mattw-nws mattw-nws mentioned this pull request Dec 21, 2022
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattw-nws mattw-nws mattw-nws approved these changes

@hellkite500 hellkite500 Awaiting requested review from hellkite500

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Example realization file tune-up/catch-up after #448 and #430
2 participants
@stcui007 @mattw-nws