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

[1pt] PR: Remove GMS branch outlet backpools #1006

Merged
merged 54 commits into from
Mar 11, 2024

Conversation

EmilyDeardorff
Copy link
Contributor

@EmilyDeardorff EmilyDeardorff commented Oct 11, 2023

In some HUCs, an overly-large catchment appears at the outlet of the branch (as in issue #985). This catchment causes an artificially large amount of water to get routed to the smaller stream instead of the main stem, which means more of the water that could be causing flooding is coming from the small stream instead of going directly into the larger river. This issue can be mitigated by trimming the levelpath just above the outlet and removing the offending pixel catchment from the pixel catchments and catchment reaches files.

The branch outlet backpool issue is identified based on two criteria:

  1. There is a pixel catchment that is abnormally large (more than two standard deviations above the mean.)
  2. The abnormally-large pixel catchment occurs at the outlet of the levelpath.

If both criteria are met for a branch, then the issue is mitigated by trimming the flowline to the third-to-last point. The script also reads in the pixel catchments raster and the reaches raster and removes the inforrectly large outlet pixel catchment. This mitigates the branch outlet backpool error.

Additions

  • src/mitigate_branch_outlet_backpool.py
    • Detecting the branch outlet backpool error:

      • Added the catchment pixels file as an input to the main function
      • Added the catch_catchment_size_outliers() function to catch catchment size outliers in the catchment pixels layer
      • Added the get_raster_value() function to extract the catchment ID from the catchment_pixels geometry for a point feature (branch outlet backpool criteria 1)
      • Added the check_if_ID_is_outlet() function to test whether any of the outlier catchments are occurring at the levelpath outlet (branch outlet backpool criteria 2)
    • Mitigating the branch outlet backpool error:

      • If both branch outlet backpool criteria are met, the snapped point is set to be the penultimate vertex and then the flowline is trimmed to that point (instead of the last point).
      • Trim the gw_catchments_pixels_<id>.tif and gw_catchments_reaches_<id>.tif rasters by using gdal_polygonize.py to polygonize the gw_pixel_catchments_<id>.tif file, creating a mask that excludes the problematic pixel catchment, and then using that mask to trim the pixel catchment and catchment reaches rasters.

Changes

  • src/delineate_hydros_and_produce_HAND.sh

    • Add the mitigate_branch_outlet_backpool.py module to run after the Gage Watershed for Pixels step.
  • src/split_flows.py

    • Documentation and readability improvements:
      • Improved documentation of script inputs at beginning of script
      • Added comments to better describe processing steps
      • Added headers to code sections

Testing

  • Tested updates on HUC 13080002.

Issuer Checklist (For developer use)

You may update this checklist before and/or after creating the PR. If you're unsure about any of them, please ask, we're here to help! These items are what we are going to look for before merging your code.

  • Informative and human-readable title, using the format: [_pt] PR: <description>
  • Links are provided if this PR resolves an issue, or depends on another other PR
  • If submitting a PR to the dev branch (the default branch), you have a descriptive Feature Branch name using the format: dev-<description-of-change> (e.g. dev-revise-levee-masking)
  • Changes are limited to a single goal (no scope creep)
  • The feature branch you're submitting as a PR is up to date (merged) with the latest dev branch
  • pre-commit hooks were run locally
  • Any change in functionality is tested
  • Passes all unit tests locally (inside interactive Docker container, at /foss_fim/, run: pytest unit_tests/)
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • CHANGELOG updated with template version number, e.g. 4.x.x.x
  • Reviewers requested
  • Add yourself as an assignee in the PR as well as the FIM Technical Lead

Merge Checklist (For Technical Lead use only)

  • Update CHANGELOG with latest version number and merge date
  • Update the Citation.cff file to reflect the latest version number in the CHANGELOG
  • If applicable, update README with major alterations

@EmilyDeardorff EmilyDeardorff added the bug Something isn't working label Oct 11, 2023
@EmilyDeardorff EmilyDeardorff self-assigned this Oct 11, 2023
@EmilyDeardorff EmilyDeardorff linked an issue Oct 11, 2023 that may be closed by this pull request
@EmilyDeardorff EmilyDeardorff marked this pull request as ready for review October 11, 2023 21:14
@CarsonPruitt-NOAA
Copy link
Collaborator

CarsonPruitt-NOAA commented Oct 13, 2023

I received this error when testing:

-----------------------------------------------------------------
Split Derived Reaches 13080002 6073000086
Thu Oct 12 20:30:52 UTC 2023
Loading data ...
Trimming DEM stream to NWM branch terminus...
Traceback (most recent call last):
  File "/foss_fim/src/split_flows.py", line 574, in <module>
    split_flows(**args)
  File "/foss_fim/src/utils/shared_functions.py", line 193, in wrapper
    func(*args, **kwargs)
  File "/foss_fim/src/split_flows.py", line 290, in split_flows
    if outlet_flag == True:
UnboundLocalError: local variable 'outlet_flag' referenced before assignment

You should either instantiate the variable outlet_flag = False or make an else statement following the if block starting on line 280:

        if flagged_catchment == True:
            print('Flagged catchment(s) detected. Testing for second criteria.') 
            outlet_flag = check_if_ID_is_outlet(snapped_point, outlier_catchment_ids)

@EmilyDeardorff
Copy link
Contributor Author

EmilyDeardorff commented Oct 13, 2023

An unforseen issue is that gw_catchments_reaches_$current_branch_id.tif, which is an input to the new processing presented in this PR, actually is created in the step AFTER split_flows.py is run in delinate_hydros_and_produce_HAND.sh (in the section titled GAGE WATERSHED FOR REACHES, which runs gagewatershed from tauDEM).

In order to move forward, we have to decide whether between a few options:

  1. Adjust the processing order so the catchment_pixels file is available for split_flows.py processing
    -> Probably wouldn't make sense because other outputs of the split_flows.py script are used as input.
  2. Use a different catchment/catchment pixels file to find outlier catchments
    -> Could work, but only if a suitable replacement file is available.
  3. Add the backpool error detection and mitigation steps to occur AFTER the GAGE WATERSHED FOR REACHES section
    -> We would just want to be careful that we don't overwrite anything added to those files. One option would be to add this functionality into split_flows.py, another option would be to put it in a different python file entirely.

@CarsonPruitt-NOAA
Copy link
Collaborator

I'm thinking that option 2 might be the best here. The more I think about our current strategy (outlier pixel catchments), the more I think that we could be introducing errors in other places. I'll make a tagup for us on Monday to discuss.

from utils.shared_functions import getDriver, mem_profile
from utils.shared_variables import FIM_ID

@mem_profile
Copy link
Contributor

Choose a reason for hiding this comment

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

@EmilyDeardorff This just caught my eye. The @mem_profile decorator function has been removed in the latest dev branch. You would probably catch this after merging the latest dev, but figured I'd give you a heads up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this! Good catch,

@EmilyDeardorff
Copy link
Contributor Author

Comparison Map

I've run our current dev version and the dev-branch-outlet-backpools branch on HUC 13080002, which is the original HUC where this issue was pinpointed. Below is a visual comparison of inundation in branch 6077000088 from dev (on the left) compared to the dev-branch-outlet-backpools branch. This figure shows that the branch outlet backpools module mitigates the backpooling effect observed on the left. The middle panel shows the section of the branch catchments that was was causing the backpooling issue and was therefore removed (depicted by cross-hatching).

Backpools Before and After

@CarsonPruitt-NOAA CarsonPruitt-NOAA merged commit 4907da2 into dev Mar 11, 2024
1 check passed
@CarsonPruitt-NOAA CarsonPruitt-NOAA deleted the dev-branch-outlet-backpools branch March 11, 2024 19:38
@mluck
Copy link
Contributor

mluck commented Mar 19, 2024

I'm wondering why there's a difference in inundation between the left and right images below. Assuming the discharge is the same, are these from different FIM versions or is the backpool algorithm doing something here?
image

@CarsonPruitt-NOAA
Copy link
Collaborator

There absolutely is a difference in the inundation that you can see in that map. However, I would expect it to go the opposite way - for there to be more inundation after the backpool fix due the change in SRC seen here. This is a good catch, Matt. It's still curious because we tested this FB before merging and there wasn't any difference in skill metrics.

@EmilyDeardorff For both of these maps, are you using the standard, composited branch zero/GMS inundation?

@mluck
Copy link
Contributor

mluck commented Mar 19, 2024

Since we were trying to find the source of the errors between 4.4.9.0 and 4.4.13.1, I ran two BLE HUCs (12040101 and 12090301) and it looks like this FB did change the CSI in both of the HUCs I looked at (red = decrease, green = increase). The number of branch errors (exit status 61) in two HUCs (09010004 and 12100303) also seem to have increased in this FB.
image

@mluck
Copy link
Contributor

mluck commented Mar 19, 2024

The blue areas (a-c) are areas of inundation that were removed by this FB, while the orange (d) was added. I thought that this FB only works at the end of levelpaths but it appears that this is also removing area along levelpaths. It looks like maybe this is because it's using the undissolved levelpaths?
image

@CarsonPruitt-NOAA
Copy link
Collaborator

We're using NextDownID == '-1' to find the terminal segment of each level path (See here). Maybe the segments you identified have a NextDownID of -1?

@mluck
Copy link
Contributor

mluck commented Mar 20, 2024

I took a closer look at the four places I pointed out earlier. It seems that this PR is doing what it should with regards to inundation (and this is likely affecting the metrics but not sure about the increase in branch errors). Here are some detailed inundation images (left = before, right = after this PR). The red line in the first two images below are DEM-derived streams with NextDownID == -1.

My only remaining question is why in image (a) and the Laredo example (that I pointed out above) are the differences in inundation above the last reach, not in the last reach? At first I thought it was because the points might be digitized in the opposite direction (downstream to upstream) but it seems like that's not the case since the large catchment area on the mainstems are being removed correctly, while it seems that the change in inundation at (d) happens at the last reach.

(a) green is the inundated area removed by the PR
image

(b), (d)
image

(c)
image

@mluck
Copy link
Contributor

mluck commented Mar 22, 2024

In the example (a) above, the SRC for the last (outlet) reach is changed slightly but the SRC of the penultimate reach is very different and would seem to account for the decreased inundation in 25070004 (lighter shaded blue). The catchments are the same before and after this FB.
image

SRC for the last (outlet) reach (25070005)
image

SRC for next reach upstream (25070004)
image

@mluck
Copy link
Contributor

mluck commented Mar 22, 2024

This is the effect on the agreement raster. Interesting that the next-to-last reach was initially over-inundated.

Before (4.4.12.0)
image

After (4.4.13.0)
image

@CarsonPruitt-NOAA
Copy link
Collaborator

Hmm yeah, that's a very significant over-inundation on the Before map. I was looking at the mitigate_branch_outlet_backpool code and saw that there is a recalculation of slope. Can you check slope and the other manning's parameters to see what (if anything) has changed between the 2 versions?

@mluck
Copy link
Contributor

mluck commented Mar 22, 2024

All of the Manning's parameters are identical between the two versions. The only thing that is changing in the next to last reach is the spatial calibration coefficient. Here's the comparison of the highest stage in 4.4.12.0 (top) and 4.4.13.0 (bottom):
image

@CarsonPruitt-NOAA
Copy link
Collaborator

@RyanSpies-NOAA Can you take a look at why the spatial calibration coefficient is so different in the newest version? This is HUC 12090301 LID brtt2 and I believe the feature_ID for the catchment in question is 5789218.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[21pt] Remove GMS branch outlet backpools
6 participants