Update split_by_square dependency from fmtm-splitter to area-splitter - #820
Update split_by_square dependency from fmtm-splitter to area-splitter#820kygia wants to merge 5 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CONTRIBUTING.md document and I hereby sign and agree with the guidelines |
Signed-off-by: kygia <hi@kygiaha.com>
for more information, see https://pre-commit.ci
spwoodcock
left a comment
There was a problem hiding this comment.
Hi! Thanks so much for taking the time 😃
The objective of replacing the current splitter was to remove the shapely dependency and rely on the PostGIS implementation.
Without shapely being replaced there is very little value to this change.
Do you have any thoughts on how the no-fly zone logic could be replaced at the same time? (we need to handle splitting by square, but with the no fly zones omitted).
Thanks again 🙏
|
Also, sorry, I realised I commented a month ago that adding no fly zone handling may be out of scope. But looking into the changes in the PR and actually seeing the code, it's become clearer to me that without handling this, then the change is probably actually a regression rather than an improvement (increased complexity with little gain) 😄 |
What type of PR is this? (check all applicable)
Related Issue
Fixes #759
Describe this PR
Replaces fmtm-splitter with area-splitter, uses PostGIS SQL and requires a DB connection instead of pure python. Updated call site and mocked _area_split_by_square in tests accordingly.
AI Tool Usage
If AI-assisted:
Screenshots
If applicable.
Alternative Approaches Considered
Did you consider other approaches? Why this one?
Review Guide
Core change is in the split_by_square call site, verify the psycopg connection is passed correctly. Test mock _area_split_by_square to avoid a live DB dependency
Checklist
[optional] What gif best describes this PR or how it makes you feel?