search

norman-tom / pyromb

Pyromb is a python package to convert a catchment diagram developed in QGIS, or any GIS package, to a RORB or WBNM control file
MIT License
6 stars 3 forks source link

Use shapely to match centroid to catchment polygon #16

Open Githubcopilot111 opened 1 month ago

Githubcopilot111 commented 1 month ago

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.

norman-tom commented 1 month ago

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.

Githubcopilot111 commented 1 month ago

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 commented 1 month ago

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.