fix: force right-hand-rule winding on AOI geometry - #7322
Conversation
Related to hotosm#4225. Assisted-by: Claude Signed-off-by: Brian Bergstrom <dulcetberg@gmail.com>
|
There was a problem hiding this comment.
Small change - I think this is good to merge 👍
Note that @dulcetberg and I have already discussed AI-assisted contributions in this thread: hotosm/drone-tm#872 (comment)
This PR includes some human reviewed tests done, that I think satisfy the criteria.
From https://responsibleai.guide/managing-ai-contributions:
I would say this is probably a 4 on the scale. Feel free to correct me 🙏
|
I want to challenge you on the rating, as I overlooked the entire scale, admittedly. The AI helped me write the code, but the critical thinking parts and decisions to test the code in Jupyter Notebook were all mine. The PR itself was written in my own words, and I understood the main concept involved with the right hand rule. Level 2, according to the image says "I planned the change and decided the approach." Level 2 does not forbid AI writing all the code. Level 3 was what I was previously doing with the prompts, not looking at the code at all. This time I actually took the code from the AI, ran it myself and came up with an alternative approach and it failed, then AI gave me the correct code and approach which I approved was the best solution to commit. Furthermore, I manually did all the git commands, not letting AI submit everything like before. It was for those reasons, I feel this should be at Level 2, not 4. |
|
Sounds good to me, thanks for clarifying! It's on us for not including the rating in the PR template for Tasking Manager yet. The latest revision of the responsible AI guide was just released, so we are due to implement a few things across multiple tools 👍 |
2 participants
Related to #4225.
Assisted-by: Claude
What type of PR is this? (check all applicable)
Related Issue
Related to #4225
Describe this PR
I investigated this issue in Jupyter Notebook trying to reproduce the results given in Shapely. From the gist linked to the issue, I tested it out in Shapely to see if it still worked. What I discovered was it parses fine in Shapely (is_valid: True), but the exterior ring is still wound clockwise instead of counter-clockwise. This is in violation of the right hand rule convention which states the ring around a polygon must be viewed counter-clockwise and a ring (such as a lake) inside the polygon should go clockwise.
What I cannot confirm, without access to the original Sentry logs, is exactly which part of the processing chain triggers the actual reported error ('Polygon' object does not support indexing). What I can confirm directly was the winding-order anomaly @spwoodcock flagged was real and still present in this data. I applied ST_ForceRHR() which is a low risk normalization that removes said anomaly regardless if that is the cause of the original crash.
Following the fix @spwoodcock proposed which was to wrap the geometry with ST_ForceRHR() before it is actually stored, forcing the correct winding order. The code has been refactored since his original sketch with parameterized SQL instead of SQLAlchemy function objects. This means that no wrapper function is needed in utils.py, I just added it directly to the existing query.
Testing : I could not run the actual ST_ForceRHR PostGIS function directly because I have no live database available to me. What I did try was DuckDB's spatial extension first since I already had that locally installed in Jupyter. Turns out that did not work because DuckDB does not implement ST_ForceRHR. My next step was to try it in shapely.geometry.polygon.orient and confirmed it flips the geometry's exterior ring from clockwise to counter-clockwise, which matches what ST_ForceRHR does in PostGIS. Here is my screenshot of what I produced in Jupyter Notebook:
Screenshots
Alternative Approaches Considered
Tried DuckDB's spatial extension first, since I had it available locally. I confirmed it doesn't implement ST_ForceRHR (real error, not an assumption — see screenshot 2). Fell back to Shapely's orient() as a proxy verification of the winding-order fix, since I don't have a live PostGIS instance to test the actual function directly.
Review Guide
No live database available to me to test ST_ForceRHR() directly against real PostGIS. The change itself is a single added SQL function wrapper — reviewer testing against a real DB with the failing geometry from the linked gist would be the strongest confirmation.
AI Disclosure
If checked, briefly describe what AI assisted with (e.g., code generation, refactoring, tests, documentation) and confirm you have reviewed and understood the changes.
Claude helped me understand the right-hand-rule concept, scope the fix, and draft this description for me to edit. All testing — the Shapely reproduction, the DuckDB attempt, and the orient() verification — I ran myself in my own Jupyter notebook (screenshots attached). I've read through the change and understand what it does.
Checklist before requesting a review
[optional] What gif best describes this PR or how it makes you feel?