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

Feat sagehen domain #288

Merged
merged 43 commits into from
Apr 24, 2024
Merged

Conversation

jmccreight
Copy link
Member

@jmccreight jmccreight commented Apr 6, 2024

  • Checks the first item in implement GSFLOW/PRMS cascades #173
  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst or it's sub rsts?
  • Update/add test_data/README.md with domain descriptions

This PR adds the sagehen_5yr domain that will be used to test the addition of cascades to pywatershed. This sagehen_5yr domain is a HRU/polygon/unstructured domain. A sagehen gridded domain will be added later. The configuration of this domain differs from the NHM configuration implemented by pywatershed so far in several significant ways listed below. This PR includes the inclusion of the following functionality to support the sagehen_5yr domain WITHOUT cascades:

  1. "No depression storage" subclasses of PRMSRunoff, PRMSSoilzone, and PRMSGroundwater. The sagehen_5yr domain does not run depression storage. We note that the implement is a efficient but potentially bad/wrong and should/could be revisited to close redesign depression storage #280 and during the implementation/subclassing to add cascades. The sagehen_5yr CI tests cover these classes in addition to the tests added in PRMSRunoff working without depression storage #279 with the nhm_no_dprst.control configurations of the drb_2yr, ucb_2yr, and hru_1 domains.
  2. dunnian_flow is active in this domain whereas it is not in the NHM configurations/parameters because sat_threshold is set to 999 for NHM configurations. dunnian_flow means that sroff is set by both PRMSRunoff and PRMSSoilzone with sroff being an input to PRMSSoilzone. The AdapterNetcdf class handles modification of input just fine, as it passes the pointer around. The test_prms_runoff is skipped when the minumum of sat_threshold is less than 999 because runoff maybe altered in PRMSSoilzone which the test is not designed to cover. Additionally, the new test test_prms_below_snow verifies that dunnian flow is computed properly when it is active.
  3. Preferential flow is off in the NHM configuraton as pref_flow_den is zero but it is active in sagehen_5yr. The pref_flow_infil_frac was missing from the metadata and has been added. All the code is now fully implemented for preferential with this PR and is tested in CI by sagehen_5yr domain tests.
  4. Something, likely in the parameters, is triggering a section of code in PRMSCanopy that sets pptmix to zero when hru_snow > 0 and netsnow < nearzero. This apparently never happened with the NHM configuration. This is the modification of an input/AdapterNetcdf, as in 2, where pptmix is a PRMSAtmosphere variable and an input to PRMSCanopy where it is altered.. The existing test_prms_atmopshere was easily modified to make the modifications to pptmix before checking the answers this should print a warning. The new test test_prms_above_snow checks that pptmix is calculated correctly and set in both processes.

Bug fixes:

  1. transp_on in PRMSAtmosphere was improperly implemented but the bug was never triggered by the NHM configuration. This has been fixed.
  2. dprst_area_open was incorrect in PRMSRunoff and was not being checked (it apparently has no affect on other variables). It has been fixed and is being checked.

Features:

  1. Tests are now marked as "domain" or "domainless" to avoid redundancy in testing.

Copy link

codecov bot commented Apr 6, 2024

Codecov Report

Attention: Patch coverage is 59.66387% with 96 lines in your changes are missing coverage. Please review.

Project coverage is 72.27%. Comparing base (dcaedbb) to head (0b593bc).

Files Patch % Lines
pywatershed/utils/utils.py 8.00% 23 Missing ⚠️
pywatershed/hydrology/prms_runoff_no_dprst.py 57.14% 21 Missing ⚠️
pywatershed/hydrology/prms_soilzone_no_dprst.py 60.52% 15 Missing ⚠️
pywatershed/hydrology/prms_groundwater_no_dprst.py 57.57% 14 Missing ⚠️
pywatershed/hydrology/prms_soilzone.py 40.90% 13 Missing ⚠️
pywatershed/base/control.py 80.76% 5 Missing ⚠️
pywatershed/base/process.py 88.23% 2 Missing ⚠️
pywatershed/hydrology/prms_canopy.py 0.00% 1 Missing ⚠️
pywatershed/parameters/prms_parameters.py 50.00% 1 Missing ⚠️
pywatershed/utils/separate_nhm_params.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #288      +/-   ##
===========================================
- Coverage    75.70%   72.27%   -3.44%     
===========================================
  Files           50       53       +3     
  Lines         6644     6839     +195     
===========================================
- Hits          5030     4943      -87     
- Misses        1614     1896     +282     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

* remove test_model in favor of test_model_above_snow and test_model_below_snow
* add new forcing and yaml files for sagehen_5yr
@jmccreight jmccreight merged commit 243b5de into EC-USGS:develop Apr 24, 2024
10 of 12 checks passed
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.

redesign depression storage
1 participant