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

V5.1.0 lake fixes #161

Merged
merged 4 commits into from
Aug 28, 2018
Merged

V5.1.0 lake fixes #161

merged 4 commits into from
Aug 28, 2018

Conversation

aubreyd
Copy link
Collaborator

@aubreyd aubreyd commented Aug 28, 2018

Committing a few lake fixes:
(1) Properly enclosing weir equation exponent in parentheses.
(2) Setting uniform lake surface area consistent with vertical wall assumption (may change in the future).
(3) Zeroing out qlateral inputs for reach-based methods for now as this was likely improperly specified (qlateral inputs are more important for gridded).
(4) Yates' updates to Runge-Kutta solution formulation.

We left in a quick & dirty diag water budget tracking that is commented out for now. Won't get full closure at any given sub-timestep due to approximate RK solution, but should even out to 0s. This should eventually be moved out of the channel routing timestep.

Engineering tests passed. Expected answer changes in channel and lake fluxes only. These answer changes over a multi-year CONUS run were evaluated offline by the Wonderful Read.

Aubrey Dugger added 4 commits August 24, 2018 12:58

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…area, and zero out lateral inflow (temporarily). Prints needs to be removed beofre primetime.
@@ -94,7 +94,7 @@ subroutine LEVELPOOL(ln,qi0,qi1,qo1,ql,dt,H,ar,we,maxh,wc,wl,oe,oc,oa)
real :: dh, dh1, dh2, dh3 ! Depth in weir, and height function for 3 order RK
real :: It, Itdt_3, Itdt_2_3 ! inflow hydrographs
real :: maxWeirDepth !maximum capacity of weir
real :: qldum !dummy ql to 0 out
!real :: hdiff_vol, qdiff_vol ! water balance check variables
Copy link
Contributor

Choose a reason for hiding this comment

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

If adding new variables to the code, can we make an effort to move away from F77 style/conventions? Pick a descriptive name and add a descriptive inline comment with units. Is qdiff_vol a flow? A Rate? A Volume? Similiarly, hdiff_vol is not informative, I can't determine if it is a height, head, or volume. I recommend adding them each on their own line with elaborated names and inline comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, although these were really just our internal testing checks that I left in just in case we need to activate again. This is not really how we should be doing mass balance checks on this routine, which is why it is commented out. Might be cleaner just to remove completely - this is really just a quick patch to get these updates into ops since I know you all are working on a full refactor for next round...

@tjmills
Copy link

tjmills commented Aug 28, 2018

@laurareads can you include some more details on your offline tests/evaluation mentioned above by @aubreyd ?

@laurareads
Copy link
Contributor

We have tested these changes across CONUS, evaluating at all USGS gauge locations for a 4-year period. Statistics do not show alarming differences, and answer changes are within expected ranges. The testing results shown here (answer diffs) are expected since the lake code changes will result in different behavior while the model is spinning up/lakes reaching equilibrium.

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

Successfully merging this pull request may close these issues.

None yet

4 participants