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

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

Merged
merged 25 commits into from
Jul 16, 2024

Conversation

saubhagya-gatech
Copy link
Contributor

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.

…function

- to define the width, any property other than stream order can be defined through function argument, examples, width based on drainage area, labels in node properties, distance from the outlet, etc.
- this is more robust, especially than the previous method.
- Previous method is available as densify_river old. This will be depreciated eventually once new method is test enough times
@saubhagya-gatech
Copy link
Contributor Author

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 saubhagya-gatech changed the title WIP: New densification methods, variable quad length-scales. increased flexibility for defining quad widths, depths New densification methods, variable quad length-scales. increased flexibility for defining quad widths, depths Mar 22, 2024
@saubhagya-gatech
Copy link
Contributor Author

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

For the case when the small segment is one of the tributaries at the junction, we merge it with its child (if there is only one child) and not with its parent.
@saubhagya-gatech
Copy link
Contributor Author

saubhagya-gatech commented Apr 18, 2024

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')

@ecoon
Copy link
Collaborator

ecoon commented Apr 22, 2024

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).

The property codes in NHD Datasets are in lowercase. The code is updated to change property codes to lowercase in cases the new format of the NHD Dataset is encountered. This change is applicable for not only 'NHDFlowline' layer, but also in layers 'NHDPlusCatchment', 'NHDPlusFlowlineVAA',
 NHDPlusEROMMA'.
Two tests were failing; this could be due to updates in the hydrography in the new NHD dataset. All other tests are passing fine, and all example problems are also running fine.

FAILED watershed_workflow/test/test_hilev.py::test_river_tree_properties - assert 94 == 97
FAILED watershed_workflow/test/test_hilev.py::test_river_tree_properties_prune - assert 49 == 50
saubhagya-gatech and others added 3 commits April 24, 2024 13:13
We just keep the node to be merged as self, and , merge it into parent or child based on a flag.
@ecoon ecoon merged commit 3d80709 into master Jul 16, 2024
1 check passed
@ecoon ecoon deleted the ssr/improve_stream_aligned_meshing branch July 16, 2024 18:30
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.

2 participants