-
Notifications
You must be signed in to change notification settings - Fork 4
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
giuh_time_scaling #10
Conversation
…imestep of 5 minutes.
@PhilMiller Thanks Phil for your suggestions, they have been incorporated. please have a look and see if you have more suggestions. Although I added you as a reviewer by mistake, it turned out to be a useful mistake. Appreciate it! |
You're welcome. I'm happy to help. And no worries, you didn't accidentally add me as a reviewer - I'm a newcomer to the NWM enterprise, subscribed to activity on a bunch of the NOAA-OWP repositories. I skim whatever PRs I see come up as a way of gaining more fluency with the overall codebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't speak to the substance of the change, but the issue I saw is addressed, so no longer 'Request changes'
I tested it in ngen and standalone, both worked well. I played around with the GIUH ordinates and they did what I expected them to. Everything looks good. |
And thanks Phil for the suggestion! |
The PR fixes GIUH time scaling reported in the issue. The giuh ordinates from percentage per hour are scaled to account for LGAR model timestep.
Additions
Removals
Changes
Testing
Screenshots
The giuh runoff using the old code has a high peak and quick recession, while the new code (taking into account time scaling) has a lower peak and relatively smooth rising and falling limbs, which makes sense.