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

Add lake_option to hydro_namelist, reservoirs to their own new namelist #725

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rcabell
Copy link
Collaborator

@rcabell rcabell commented Nov 10, 2023

TYPE: enhancement

KEYWORDS: lakes, reservoirs

SOURCE: NCAR

DESCRIPTION OF CHANGES:

  • Add lake_option (integer) to &hydro_nlist: 0 [lakes off], 1 [level pool], or 2 [passthrough], or 3 [reservoir DA]

    • turning lakes off (lake_option=0) will disable lakes even if route_lake_f is supplied, or outlake is turned on.

    • Reservoir DA will not be used unless lake_option=3, even if all other required namelist options are present

  • Reservoir options have been moved from &hydro_nlist to &reservoir_nlist

    • This will make it easier to isolate / compose namelist files

    • If lake_option is not equal to 3, &reservoir_nlist won't be read, meaning it can be completely removed for applications that don't need it

TESTS CONDUCTED: In-progress, will convert from Draft and update here when complete.

@rcabell
Copy link
Collaborator Author

rcabell commented Nov 10, 2023

CI Tests failing due to namelist changes. Will update...

@arezoorn
Copy link
Contributor

I performed the following tests with channel only runs for one to two months.

  • Confirm the cold start with level pool give identical answers - Done and confirmed it produced identical results
  • Confirm that warm start with level pool give identical answers - Done and confirmed it produced identical results
  • Confirm that cold start with no lake give identical answers - Done and confirmed it produced identical results.
  • Confirm that a warm start with no lake gives identical answers- Done and confirmed it produced identical results.
  • Confirm that a warm start with a pass-through option works fine - the answers would not be the same but would like to make sure there are no strange values in it. Done and hydrographs at hundreds of locations were reviewed and they look normal.

@rcabell rcabell force-pushed the lakeopt-nml branch 2 times, most recently from 0896510 to b56ef84 Compare November 14, 2023 17:10
@NCAR NCAR deleted a comment from hydro-automerge Nov 14, 2023
@rcabell rcabell marked this pull request as ready for review November 14, 2023 17:23
scrasmussen
scrasmussen previously approved these changes Nov 14, 2023
Copy link
Member

@scrasmussen scrasmussen left a comment

Choose a reason for hiding this comment

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

Read through everything and it LGTM!

@rcabell
Copy link
Collaborator Author

rcabell commented Dec 7, 2023

I added a small fix to config.f90 to reset lake_option from 3 to 1 (level pool) after setting the reservoir_type_specified flag (which activates the various Reservoir DA schemes). I also added a message to LEVELPOOL_PHYSICS() to call hydro_stop() if an invalid lake_option is passed to it.

This fix corrects the regression seen on Derecho when using Reservoir DA [not tested on GitHub CI].

scrasmussen
scrasmussen previously approved these changes Dec 7, 2023
Copy link
Member

@scrasmussen scrasmussen left a comment

Choose a reason for hiding this comment

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

Those fixes and changes look good!

* Add `lake_option` (integer) to &hydro_nlist:
  0 [lakes off], 1 [level pool], or 2 [passthrough], or 3 [reservoir DA]

  - turning lakes off (lake_option=0) will disable lakes even if
    route_lake_f is supplied, or outlake is turned on.

  - Reservoir DA will not be used unless lake_option=3, even if all
    other required namelist options are present

* Reservoir options have been moved from &hydro_nlist to
  &reservoir_nlist

   - This will make it easier to isolate / compose namelist files

   - If lake_option is not equal to 3, &reservoir_nlist won't be read,
     meaning it can be completely removed for applications that don't
     need it
Copy link
Member

@scrasmussen scrasmussen left a comment

Choose a reason for hiding this comment

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

Looks good to me! Did some testing and I things seemed to function properly

@@ -201,7 +201,7 @@ subroutine hybrid_init(this, water_elevation, &
! Initialize level pool reservoir
call this%state%levelpool_ptr%init(water_elevation, lake_area, &
weir_elevation, weir_coeffecient, weir_length, dam_length, orifice_elevation, &
orifice_coefficient, orifice_area, lake_max_water_elevation, lake_number)
orifice_coefficient, orifice_area, lake_max_water_elevation, lake_number, 1)
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine either way but do we want to make this 1 and the 1 in module_rfc_forecasts.F90:159 a named constant to better indicated what is being passed?

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

Successfully merging this pull request may close these issues.

3 participants