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: Unmask leveed areas update #1136

Merged

Conversation

mluck
Copy link
Contributor

@mluck mluck commented Apr 30, 2024

Levee-protected areas are associated with levelpaths based on a 1000 m buffer on each side of the levee line. However, not all levees are designed to protect against all associated levelpaths, especially where the levelpath flows through the levee-protected area. Levee-protected areas are unmasked by removing levelpaths from association that don't intersect levees but instead flow around them which allows inundation by these branches.

Fixes #1124.

Changes

  • src/associate_levelpaths_with_levees.py: Finds levelpaths that don't intersect levees and removes them from their association with their levee-protected area.

Testing

  • Needs significant testing (watching .csv files)

Deployment

  • Run its own UAT set on it.
  • Aim for next big 4.5 prod push but can be held over if necessary (try to get it in)

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

@mluck mluck added bug Something isn't working FIM4 labels Apr 30, 2024
@RobHanna-NOAA
Copy link
Contributor

This ran perfectly fine in the processing part of the fim-pipeline. It failed in post-processing but it is unrelated. If you know of a couple of HUCs that have levee data, this can be validated.

@mluck
Copy link
Contributor Author

mluck commented May 10, 2024

I merged dev (v.4.5.0.1) and ran fim_pipeline.sh on two leveed HUCs (10300101 and 12030105) and it ran fine including post-processing. Of course those HUCs may not have the problem but at least those two cases seem to have worked.

@ZahraGhahremani
Copy link
Contributor

ZahraGhahremani commented May 14, 2024

After comparing the new results with the previous ones, it was found that 174 levelpaths out of 1868 were removed in the new run. Although the inundation results demonstrate success in most areas, there are cases where a protected area intersects the levee lines (as shown in the figure below). In these cases, the levee system should not be unmasked, but it was unmasked.

huc21010005

@CarsonPruitt-NOAA
Copy link
Collaborator

Zahra, your example appears to show a flowline that intersects a levee. Isn't this PR only addressing protected areas that do NOT intersect levee lines? If so, this levee system should remain masked.

@mluck
Copy link
Contributor Author

mluck commented May 14, 2024 via email

@mluck
Copy link
Contributor Author

mluck commented May 15, 2024

The latest commit (439f0e9) fixes the bug that was masking the levee-protected area if the levelpath intersects the levee multiple times. However, there is still the possibility that this FB may unintentionally unmask inundation where a levelpath doesn't cross a levee line but does cross through a levee-protected area.

In the example below, the levee-protected area in yellow is associated with the levee in red. The red levee is protecting agains the green levelpath near its downstream confluence with the purple mainstem and doesn't intersect the red levee line. However, further upstream, the green levelpath crosses the yellow levee-protected area above the red levee. The green levelpath appears to be erroneous since it also crosses a different levee at this location. In addition, the yellow leveed area appears to be a bit too large and extends into the channel beyond this other levee where the green levelpath also crosses it. This FB would unmask the yellow leveed area and allow inundation by the green levelpath in the yellow leveed area, even behind the red levee. More investigation is being done to evaluate the extent of this issue and if it causes more problems than it solves.

image image

@ZahraGhahremani
Copy link
Contributor

It is difficult to compare the new results with fim_4_4_15_0 due to discrepancies in folder names. Example below shows the levelpath IDs for huc 21010005 in fim_4_4_15_0 and the new results.
Instead, the new results were compared with fim_4_5_0_1, revealing that 36 levelpaths (from 15 unique HUCs) were removed. The metrics comparing the new results with the recent FIM version (fim_4_5_0_1) are promising. When examining the metrics individually, only two metrics have changed, both of which are among those listed with different numbers in the levee_levelpaths.csv. Therefore, this PR is unlikely to have unintended effects.
remove-levpa-2

@RobHanna-NOAA
Copy link
Contributor

We just put out 4.5.2.0. Is there merit in merging the latest dev into this branch and re-running this branch? Does that give you new material to compare that might be more helpful?

@CarsonPruitt-NOAA
Copy link
Collaborator

@mluck @ZahraGhahremani Since Zahra's last review, I believe you two told me that you re-checked and there were very few differences between dev and this branch's levee masks and you were able to spot-check a few levees where the results were all as intended. I just want to confirm before merging this one in. Thanks!

@ZahraGhahremani
Copy link
Contributor

Yes, that's correct. Everything looks good to me. I spot checked several levees and everything matched our expectations.

@RobHanna-NOAA
Copy link
Contributor

sorry.. correction. @mluck , can you merge latest dev into it, then Zahra can just re-approve. :)

@RobHanna-NOAA
Copy link
Contributor

RobHanna-NOAA commented Jul 17, 2024

So.. Is this one ready for approval now @ZahraGhahremani ? Actually.. we should do another quick test on it, in light of the dev merge catchup.

@ZahraGhahremani
Copy link
Contributor

So.. Is this one ready for approval now @ZahraGhahremani ? Actually.. we should do another quick test on it, in light of the dev merge catchup.

Yes!

@RobHanna-NOAA
Copy link
Contributor

after double checking it, I am pretty sure another quick test is not needed. :)

@mluck mluck changed the title [1pt] PR: Unmask leveed areas from levelpaths that intersect levee-protected areas but don't intersect levees [1pt] PR: Unmask leveed areas update Aug 2, 2024
@CarsonPruitt-NOAA CarsonPruitt-NOAA merged commit b47e4fa into dev Aug 2, 2024
1 check passed
@CarsonPruitt-NOAA CarsonPruitt-NOAA deleted the dev-remove-levelpaths-intersecting-leveed-areas branch August 2, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working FIM4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[13pt] Leveed area is so large/long that it is masking too many branches
4 participants