Use shapely to match centroid to catchment polygon by Githubcopilot111 · Pull Request #16 · norman-tom/pyromb

Githubcopilot111 · 2024-10-10T18:39:18Z

No description provided.

Githubcopilot111 · 2024-10-10T19:03:44Z

Most (all?) catchment polygons were being incorrectly assigned to centroids when one of the centroids was matching incorrectly. This resulted in some catchment areas being repeated for catchments in the CATG file, and others being omitted.

Updated to use shapely and a test to check if the centroid is within the catchment polygon to do the matching instead of the previous method.

test.catg.txt - catg showing the issue
testing_mod_python2.catg.txt - catg which appears to be fixed
BC_basins.zip - example files which cause the issue

Started on resolution to some of the open issues as part of investigation/testing this issue.

Works with my app_testing.py file, but I couldn't figure out how to get it working in QGIS.

src/app.py

norman-tom · 2024-10-13T05:05:14Z

I like the logic of having the intersection of the basin and the centroid determine assignment, it makes sense and is more robust. It does introduce the need for and additional dependency however. The GDAL library should be used for the geographic operations (i.e. intersection) however, rather than Shapely. The GDAL library is already packaged with QGIS. By using the GDAL library the dependencies will be kept to a minimum and be useful in other GIS application that already have access to GDAL.

Validation should happen by the Builder Class when building each of the reaches, confluences, centroids, basins rather than in the app.py file. This would ensure validation is within the Pyromb library and not have to be re-implemented in other interfaces such as QGIS. The app.py is purely an interface application to the Pyromb Library. It serves the same functionality as the QGIS plugin. Grabs the shapefiles, wraps them in the VectorLayer then passes them to the builder. The QGIS plugin is located https://github.com/norman-tom/runoff-model/tree/main if you haven't seen it.

norman-tom · 2024-10-13T04:35:13Z

src/app.py

+
+    # Decide whether to proceed based on validation
+    # Decide whether to proceed based on validation
+    if not all(


can remove this as the builder will check and throw an exception.

norman-tom · 2024-10-13T04:37:29Z

src/app_testing.py

Delete this file from the pull request.

src/pyromb/core/attributes/reach.py

norman-tom · 2024-10-13T04:41:39Z

src/pyromb/core/geometry/shapefile_validation.py

@@ -0,0 +1,148 @@
+from matplotlib.ft2font import SFNT


Remove matplotlib import.

Change over to the GDAL library as this will work out of the box with QGIS.

norman-tom · 2024-10-13T04:43:36Z

src/pyromb/core/gis/builder.py

-            min = 0
-            d = 999
-            s = centroid.geometry(i)
+            centroid_geom = centroid.shapely_geometry(i)


Nice, just need it with the GDAL library.

I used shapely as I'm more familiar with that. GDAL is annoying to install outside of QGIS...
Shall see what the equivalent ogr2ogr commands are.

GDAL can be installed with Conda without too much trouble. ogr2ogr is a command line tool is it not? this would require a subprocess to run which I would not want to do.

The other, probably a more appropriate way, is to create a new operation under the math.geometry module called "contains" which given two geometries returns whether one contains the other. This contains functionality can be implemented by a geoprocessing library such as GDAL. but then can also be easily reimplemented by some other library (shapely) if needed. This would ensure the external geoprocessing library is only being used in one place allowing easy override.

This is also relevant to geometry validation. There should be wrapped in the math.geometry module and then used the math.geometry module in the Builder class.

Have got it working with osgeo through app.py, but shall check during the week what is required to make it work via QGIS. I assume one of the classes is going to get angry when dealing with the QGIS classes that need need to have a geometry method converted into something ogr likes.

Have tried to keep osgeo in geometry.py only, but I think it might have spread a bit further. Not sure of the best way to take it out of the validate_shapefile_geometries function.

My understanding is that GDAL is for the raster work, OGR is for the vector work. Or those are the tools I use on the command line anyway. They are also available as python and C bindings - this is the import I am using: from osgeo import ogr

pyproject.toml

setup.py

norman-tom · 2024-10-13T04:47:04Z

src/app.py

+CENTROID_PATH = os.path.join(DIR, "../data", "centroids.shp")
+CONFUL_PATH = os.path.join(DIR, "../data", "confluences.shp")
+
+# Load expected fields from JSON file


The vector files validation should be put into the Pyromb library. The Builder Class should responsible for making the fields and geometry is satisfactory to build the objects. The intent the interface (app.py, QGIS plugin) remains light weight, only being used to feed in shapefiles. Pyromb should do the rest. This reduces the coupling between Pyromb and the calling application. When the Builder is constructed, the EXPECTED_FIELDS could be created and assigned as attribute of the Builder Object.

Moved into a new shapefile_validation.py file and added some extra validation code.
Expected fields are loaded as part of init for builder.

norman-tom · 2024-10-13T04:47:34Z

src/app.py

+        The hydrology model to use.
+    """
+    # Set default paths if not provided
+    reach_path = reach_path or REACH_PATH


Happy for the vector paths to be passed directly to the main function as this would reduce the need for global variables within main.

Changed this around a bit and added some code that was useful for testing (if a flag is set). Understand it's just for the calling of the librayr.

src/app.py

Githubcopilot111 · 2024-10-14T00:54:52Z

Validation should happen by the Builder Class when building each of the reaches, confluences, centroids, basins rather than in the app.py file. This would ensure validation is within the Pyromb library and not have to be re-implemented in other interfaces such as QGIS. The app.py is purely an interface application to the Pyromb Library. It serves the same functionality as the QGIS plugin. Grabs the shapefiles, wraps them in the VectorLayer then passes them to the builder. The QGIS plugin is located https://github.com/norman-tom/runoff-model/tree/main if you haven't seen it.

Yes, we found your QGIS plugin and are wanting to use it for a project. We ran into a few errors on the first catchment we tried so I was trying to figure out if it was user error or issues with the library (ended up being both). I had done enough work chasing things through it made sense to send through an update.

Another validation test that would be useful is to confirm that all reaches touch centroids/confluences as this was the first error we had (but was fixed after some manual checking to find vertices that were not snapped so I haven't made a python validation test for it).

I shall tweak how the script flows. I've not made a QGIS plugin before so wasn't sure how the process completely worked. Shall move more items into pyromb. This is probably why the changes did not work in QGIS for me when I tried updating the library (but did work in my own environment).

Chain-Frost · 2024-10-20T12:29:42Z

Switched from work to home account.
https://github.com/Chain-Frost/pyromb/tree/convert-to-ogr
Will test using it as a QGIS library next week - seemed to work via app.py.
Black formatter seems to like changing the line endings and other small things to generate additional changes.

…atching

Use shapely to match centroid to catchment polygon

1821f6d

norman-tom reviewed Oct 13, 2024

View reviewed changes

src/app.py Show resolved Hide resolved

norman-tom requested changes Oct 13, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into Shapely-catchment-m…

136a1f4

…atching

Conversation

Githubcopilot111 commented Oct 10, 2024

Uh oh!

Githubcopilot111 commented Oct 10, 2024

Uh oh!

Uh oh!

norman-tom commented Oct 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Githubcopilot111 commented Oct 14, 2024

Uh oh!

Chain-Frost commented Oct 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants