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

Additional Reservoir Outputs #488

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jdmattern
Copy link
Contributor

@jdmattern jdmattern commented Oct 28, 2021

The current NWM 2.1 Lake Out files have additional fields for "reservoir_type" and "reservoir_assimilated_value". These values are also needed for NWM 3.0. These values do not affect anything in the model and are only for output purposes to assist the user in evaluating Hybrid and RFC type reservoirs. These values are dynamic and output from Hybrid and RFC type reservoirs at every timestep.

These values are assembled into dataframes indexed by lake/reservoir id at these locations:

https://github.com/jdmattern-noaa/t-route/blob/additional-reservoir-outputs/src/nwm_routing/src/nwm_routing/__main__.py#L705

https://github.com/jdmattern-noaa/t-route/blob/additional-reservoir-outputs/src/nwm_routing/src/nwm_routing/output.py#L102

These additions were tested with the three different types of reservoirs in the Croton NY domain and with the CustomInput.yaml file. All cases pass fine on the Cheyenne system. On apd-dev, all of the Croton cases pass fine, but the CustomInput.yaml case interestingly does not terminate and does not pass this existing function:
https://github.com/jdmattern-noaa/t-route/blob/additional-reservoir-outputs/src/python_framework_v02/troute/nhd_io.py#L352

The CustomInput.yaml case will successfully complete though if the creation of either one of the dataframes is commented out and not created at the above link in main.py. This leads me to believe that it could be a memory issue though the domain is very small with relatively few timesteps. I recommend merging this branch and potentially investigating further. I can open an issue for this if recommended.

This PR closes #466.

@mehdire61

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
  • Visually tested in supported browsers and devices (see checklist below 👇)
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist

Target Environment support

  • Windows
  • Linux
  • Browser

Accessibility

  • Keyboard friendly
  • Screen reader friendly

Other

  • Is useable without CSS
  • Is useable without JS
  • Flexible from small to large screens
  • No linting errors or warnings
  • JavaScript tests are passing

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.

Additional reservoir outputs needed for Lake Out files
1 participant