Xinanjiang #559
Merged
Xinanjiang #559
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Looks like is passing CI at least (so w/out the new scheme active). @prasanthvkrishna @aubreyd do either of you know if this code has been tested with the new scheme active or if there's a quick test we might run? |
| 17, 11.55, 0.030, -10.472, 0.468, 0.454, 0.468, 9.74E-7, 1.12E-5, 0.030, 0.60, 'PLAYA' | ||
| 18, 2.79, 0.006, -0.472, 0.200, 0.17, 0.069, 1.41E-4, 1.36E-4, 0.006, 0.52, 'LAVA' | ||
| 19, 2.79, 0.01, -0.472, 0.339, 0.192, 0.069, 4.66E-5, 2.65E-5, 0.01, 0.92, 'WHITE SAND' | ||
| 19,1 'BB DRYSMC F11 MAXSMC REFSMC SATPSI SATDK SATDW WLTSMC QTZ AXAJ BXAJ XXAJ ' |
There was a problem hiding this comment.
@aubreyd is this something that will break the soil properties script?
This shouldn't be going into the upcoming release anyway (so an issue for later), but just figured I'd check.
There was a problem hiding this comment.
I have a new version of create_soilproperties.R that handles these new params. Not sure where to stash though since this is not tied to a release (yet).
There was a problem hiding this comment.
Awesome!
Seems like these things might pile up a bit. I wonder if it make sense to just start a branch over on the repo so that we can start accruing changes in one spot (my memory is short wrt this kind of stuff)? Happy to do so or we can just wait if you'd rather.
There was a problem hiding this comment.
@kafitzgerald @rcabell @aubreyd
I just made a working test case for the basin "06917000" to test the new scheme. I have also created the spatial parameter file. Please see this path "/glade/campaign/ral/hap/prasanth/Runoff_Task/Test_code/"
If you want to see the calibration directory, it is here "/glade/campaign/ral/hap/prasanth/Runoff_Task/CALIBRATION/XAJ". Please let me know if you have any questions.
This code is developed in the Runoff Task. We ran, default, calibrated runs with this new scheme and compared the NWM performance with the original NWM version. |
|
Regression tests pass for CONUS on Cheyenne. Do we want to add new tests for this scheme before merging? |
@prasanthvkrishna - Are all the new schemes in here or just Xinanjiang? May need documentation at some point... at least the new options and params. @rcabell - Might be worth adding a runoff_option = 7 test since that is the proposed NWM option (should work out-of-the-box with the new TBL or I can provide new soil_properties file for either CONUS or Croton). We don't have automated testing for all of the NoahMP options now, so not sure how crazy we want to go. |
@aubreyd No, this PR has just the Xinanjiang. I can provide the documentation on these runoff schemes and updates in the parameter table. |
|
I think we'll want to update the soil properties files for the Croton and CONUS test cases and activate the new option in the relevant JSON file for the NWM tests. I think we've generally done this as a follow on PR in the past w/ the first one (similar to this) just ensuring we "do no harm." |
|
Thanks all! Just went ahead and updated standard Croton test case in CI and activated new scheme activated in the NWM tests aside from Hawaii yesterday evening. Looks to be failing only where we'd expect differences with the new scheme (though another set of eyes wouldn't hurt). And we have a CONUS parameter file update so hopefully we can get those running and merge here ASAP since folks are waiting on it. That said, it'd be nice to get some documentation in the repo for this @prasanthvkrishna (this can wait for a follow on PR if needed, but should go in soon) in line with our poorly advertised contributing guidelines (see number 3 in the checklist for new features). Something along the lines of this description for the channel loss functionality would be great, but ideally in the |
|
Croton CI testing shows only data drifts (no n-cores / restart failures). If someone from the science team can take a glance through and give a thumbs up/down on whether those drifts are acceptable, that would be great. |
kafitzgerald
approved these changes
Jun 3, 2021
There was a problem hiding this comment.
Diffs look reasonable to me (w/ the runoff scheme change we'd expect diffs in a large number of states and fluxes) though I don't have well calibrated expectations here.
Given the other testing that's been done, maybe we launch the CONUS test and pull some diff plots from that if there's interest in digging in? Would need to be a quick review though - this is needed for a few things.
@kafitzgerald Thanks for the review. I will provide the documentation and a summary text for the release note. |
|
CONUS testing with Long-Range physics produces only data differences, listed below: |
|
CONUS testing with full routing physics produces only data differences, listed below: |
rcabell
approved these changes
Jun 8, 2021
|
jdmattern
pushed a commit
to jdmattern/wrf_hydro_nwm_public
that referenced
this pull request
Jun 28, 2021
* Xinanjiang runoff scheme added * spatial soil XAJ params added * change NWM tests to use new runoff scheme Co-authored-by: Katelyn FitzGerald <katelynw@ucar.edu>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TYPE: New feature
KEYWORDS: A new option for Noah MP surface runoff estimation: The Xinanjiang Runoff Scheme (XAJ)
SOURCE: Prasanth Valayamkunnath, NCAR
DESCRIPTION OF CHANGES: Changes are made only on Noah MP codes.
M trunk/NDHMS/Land_models/NoahMP/IO_code/module_NoahMP_hrldas_driver.F
M trunk/NDHMS/Land_models/NoahMP/IO_code/module_hrldas_netcdf_io.F
M trunk/NDHMS/Land_models/NoahMP/phys/module_sf_noahmpdrv.F
M trunk/NDHMS/Land_models/NoahMP/phys/module_sf_noahmplsm.F
M trunk/NDHMS/Land_models/NoahMP/run/SOILPARM.TBL
ISSUE: The GitHub Issue that this PR addresses. For issue number 123, it would be:
TESTS CONDUCTED: This scheme was developed and incorporated in the NWM runoff task. The model is tested for the selected 25 NWM calibration basins (V2.1) across the US, both with default and calibrated parameters. The streamflow results from this version of the model were discussed with the team members.
Notes: This scheme is part of the official Noah MP.
Checklist
Merging the PR depends on following checklist being completed. Add
Xbetween each of the squarebrackets if they are completed in the PR itself. If a bullet is not relevant to you, please comment
on why below the bullet.
soil_properties.ncfile with additional variables, if this scheme is active.NEWS.md