watershed-workflow icon indicating copy to clipboard operation
watershed-workflow copied to clipboard

New densification methods, variable quad length-scales. increased flexibility for defining quad widths, depths

Open saubhagya-gatech opened this issue 11 months ago • 3 comments

This pull request provides improvements to the stream-alined mixed-polyhedral meshing. The main changes include:

  • Variable quad lengths: The user can provide different target quad lengths for different reaches. This helps with improving cell aspect ratios for thinner lower-order streams.
  • Widths and Supplemental Depths: Now user can use arbitrary conditions to provide quad widths and supplemental depths. Previously, only dictionaries with stream order as keys or drainage area-based functions were accepted.
  • New densification: Leveraging shapely.LineString-based interpolations while preserving endpoints, the new method yields much more uniform quad lengths compared to the previous method. Currently, the new method is available as densify_rivers_new, hopefully in a couple of months, once tested on a range of cases, will become the default. Then we can support the old method as densify_rivers_old for reproducibility.

saubhagya-gatech avatar Mar 13 '24 14:03 saubhagya-gatech

There are some updates in progress for the convexity-enforcing function to make it more robust. Once, that is done, we can go ahead with this PR.

saubhagya-gatech avatar Mar 13 '24 14:03 saubhagya-gatech

all tests are passing on the local machine. The failing test is due to NHDPlus dataset name change

saubhagya-gatech avatar Mar 22 '24 15:03 saubhagya-gatech

cleaned up code as suggested, and checked all examples. Tests are passing on the local machine. CI is failing at NHD manager due to a property name change. As suggested by Chucho, we can add the patch below, for now; we anyway map all kinds of IDs to "ID" in the properties later in the workflow. Maybe we can bring that mapping at the outset in the source manager itself?

In manager_nhd.py after line 254

if ('NHDPlusID' not in reaches[0]['properties'].keys()) & ('nhdplusid' in reaches[0]['properties'].keys()):

    for reach in reaches:

        reach['properties']['NHDPlusID'] = reach['properties'].pop('nhdplusid')

saubhagya-gatech avatar Apr 18 '24 20:04 saubhagya-gatech

cleaned up code as suggested, and checked all examples. Tests are passing on the local machine. CI is failing at NHD manager due to a property name change. As suggested by Chucho, we can add the patch below, for now; we anyway map all kinds of IDs to "ID" in the properties later in the workflow. Maybe we can bring that mapping at the outset in the source manager itself?

In manager_nhd.py after line 254

if ('NHDPlusID' not in reaches[0]['properties'].keys()) & ('nhdplusid' in reaches[0]['properties'].keys()):

    for reach in reaches:

        reach['properties']['NHDPlusID'] = reach['properties'].pop('nhdplusid')

That's fine by me as a temporary fix. Please use "and" over "&" -- they aren't the same thing (one is integer math, the other is boolean comparison operator).

ecoon avatar Apr 22 '24 21:04 ecoon