fpdcc / ccfp-asset-dashboard

CCFP Asset Dashboard
0 stars 1 forks source link

Fix phase / asset GIS bug #235

Closed smcalilly closed 1 year ago

smcalilly commented 1 year ago

Overview

includes:

Notes

gis bug fix

The comments in that issue include a lot of background about that bug, but TLDR: the intersects query was causing a segmentation fault on posgres when trying to find the intersection of political districts and a phase's assets. This happened sometimes, but not always. To fix, I'm filtering first with the dwithin function, then doing the intersection. The dwithin function trims down the area where we want to find the intersection. I've added a comment to this PR so you can see the dwithin change, since this diff is bigger with the refactor.

Update: I've upgraded postgis in the docker container so this is fixed on local development. We're migrating to CCFP's servers, so we'll hold off on fixing this on heroku (see this comment: https://github.com/fpdcc/ccfp-asset-dashboard/issues/224#issuecomment-1549877563)

signal refactor

Also includes a refactor of the django signal and GIS processing code. This fixes #223.

I've refactored all of the GIS logic into a class. I also found that a lot of the work was duplicated across signals, so I've consolidated all of the asset-related signals into one single signal so there's not as much redundant work (see this comment in the bug issue about the inefficient, redundant code). This also makes all of the GIS processing logic easier to debug, follow, etc, since it's all in one place rather than split across signals.

Testing Instructions

smcalilly commented 1 year ago

@xmedr woof. thanks for flagging. it's the same memory error that i thought i had fixed. :(

smcalilly commented 1 year ago

@fgregg I upgraded the postgis version for local development and that upgrade GEOS. The dwithin is a workaround but it doesn't address the underlying GEOS issue. Should we merge this code as is or remove dwithin?

fgregg commented 1 year ago

yeah remove dwithin