Shattering #84

raphaellaude · 2024-09-09T03:08:29Z

Todo

Description

Fixes Shattering #13

Reviewers

Primary:
Secondary:

Checklist

Added/Updated related documentation (if applicable).
Added/Updated related unit tests (if applicable).

Screenshots (if applicable):

update deps

alembic migration script

add command for creating districtrmap

bailliekova · 2024-09-11T14:33:42Z

backend/app/models.py

+    # We'll want to enforce the constraint tha the gerrydb_table_name is either in
+    # GerrydbTable.name or a materialized view of two GerryDBTables some other way.
+    gerrydb_table_name: str | None = Field(nullable=True)
+    # Null means default number of districts? Should we have a sensible default?


(O) Sensible default hard to set globally, IMO, maybe later we are more geo-aware and can default to the num of congressional districts in a state but for now I think null is fine.

backend/app/models.py

move helpers functions into app

intial draft of shatter parent udf

but still not swapping the parents and children in shatter functions correctly getting empty set of children inserted

in the future may want to pass the districtrmap uuid to shatter map so that we don't need to enfore unique gerrydb table name

raphaellaude · 2024-09-16T00:21:02Z

@bailliekova this is ready for an intermediate review! don't have a couple last things in place but would love to get your eyes on the mostly-done backend implementation, if possible!

So much to say about this but wanted to flag a couple things:

I set districtrmap to have a unique gerrydb_table_name. This is probably not what we want since we might have multiple districtr maps rely on the same underlying views (e.g. congressional vs. local maps?). I think to do this we'd need to send the districtrmap UUID to the client and remap shatter_parents to take the districtrmap UUID and parent geoids as params rather than the layer name and params.
I need to test more edge cases. For now, the tests for the new features are basically just coverage tests. Not totally sure how to test the procedure since we'd need to hit the db again to pull the inserted records?

bailliekova · 2024-09-18T14:54:46Z

What's our thought on an un-shatterable map? Is this if the "parent" is set to block groups? or a non-nesting geometry?

bailliekova

This is looking really good

bailliekova · 2024-09-18T18:47:45Z

backend/cli.py

+            "-lco",
+            "GEOMETRY_NAME=geometry",
+            "-nlt",
+            "MULTIPOLYGON",


(q) is this changing functionality or just being more explicit/constraining the output?

It's not changing functionality, at least in a breaking way! ogr2ogr is kind of annoying in that for e.g. GeoJSON's it'll load the geometry as wkb_geometry by defaylt, so this way we make sure the data type is correct / validated on load and the geometry column name won't come in as geometry, geom, geography (though technically geography is a different UDT in PostGIS, I learned recently)

bailliekova · 2024-09-18T19:04:49Z

backend/app/sql/shatter_parent.sql

+                AND a.geo_id = ANY(parent_geoids)
+                AND edges.districtr_map = districtr_map_uuid
+        ) child_geoids
+        ON CONFLICT (document_id, geo_id) DO UPDATE SET zone = EXCLUDED.zone


(q) is this just for safety? where do we expect a conflict?

It's for safety but we shouldn't see a conflict. Do you think it's actually more safe to fail loudly in this case? I was going back and forth

bailliekova · 2024-09-18T19:20:55Z

backend/app/sql/shatter_parent.sql

+            INNER JOIN parentchildedges edges
+            ON edges.parent_path = a.geo_id
+            WHERE a.document_id = $1
+                AND a.geo_id = ANY(parent_geoids)


(o) might try scooting this into the join condition- I don't have strong intuition if it'll be a a better execution plan, definitely might be.

I'll try running EXPLAIN ANALYZE!

backend/app/models.py

raphaellaude · 2024-09-19T02:41:11Z

What's our thought on an un-shatterable map? Is this if the "parent" is set to block groups? or a non-nesting geometry?

I set the child_layer in DistrictrMap to nullable – figured this would be a good flag to the frontend whether the map is shatterable or not. WDYT?

This reverts commit c90cc5e.

This reverts commit f6bd236.

This reverts commit b4495bd.

This reverts commit 911ecbd.

raphaellaude · 2024-10-10T13:33:09Z

Seeing one subtle bug here where after coming back to the page the children seem to have been lost in the state.

subtle_shattering_bug.mp4

TS and eslint not updated to standard methods

nofurtherinformation · 2024-10-14T18:08:55Z

Seeing one subtle bug here where after coming back to the page the children seem to have been lost in the state.

subtle_shattering_bug.mp4

This is related to #119 -- I have a draft PR to address this issue (and enable redux devtools) in #120

mariogiampieri

Looks great save for the radix property, which does still allow me to click and remove parent without replacing with children. overall amazing work

app/src/app/components/ContextMenu.tsx

app/src/app/components/sidebar/GerryDBViewSelector.tsx

mariogiampieri · 2024-10-14T19:19:46Z

Screen.Recording.2024-10-14.at.3.14.47.PM.mov

I may have spoken too soon - it appears that if you shatter an unassigned block, it is not replaced by unassigned children. We should decide whether we require assignment before splitting or not and address accordingly. My sense is that we would want users to be able to shatter and then assign

bailliekova · 2024-10-15T13:58:06Z

backend/app/alembic/versions/5c4028ff26df_update_pop_udf.py

+    )
+    op.drop_constraint("districtr_map_uuid_unique", "districtrmap", type_="unique")
+    op.execute(
+        sa.text("""


oh, this is nice, I was worried about our migrations. I like this better than either having to create new files for new versions or do something very convoluted to get prior versions of functions.

bailliekova

looks great!

bailliekova · 2024-10-15T14:01:24Z

backend/app/main.py

+            ParentChildEdges.parent_path,
+        )
+        .join(Document, Assignments.document_id == Document.document_id)
+        .join(


(q) is this reasonably quick? a little nervous about having two joins just to retrieve assignments. I suppose this is only called on load, though, so it's not like we're doing it repeatedly.

This is a good question. I'm seeing ~250-300ms query execution time, ~500ms round trip locally. For UX, this happens concurrently with tile loading, which usually takes about the same time or slightly longer

Agree. I barely noticed a change if any

bailliekova · 2024-10-15T14:04:14Z

backend/app/main.py

+            DistrictrMap,
+            Document.gerrydb_table == DistrictrMap.gerrydb_table_name,
+            isouter=True,
+        )
        .limit(1)


(pp) not caused by this PR, but just noticing- limit(1) is going to prevent run time errors if we do for some reason violate uniqueness constraints, but, uh, we have uniqueness constraints on the database. I think we should be consistent-- pair result.one() with a .limit(1) each time.

Totally but I'd prefer to actually remove the limit and handle the runtime error, allowing us to send an informative error and catch bugs earlier. Bugs ofc not caused by erroneous db state-because we have our constraints in place-but in our queries

With that said, let's address this in a separate mini pr

https://github.com/districtr/districtr-v2/pull/84/files/1c855645d4aac41494c3e5a33ea9b09294b3c2be#r1799832575

nofurtherinformation · 2024-10-15T15:37:13Z

Screen.Recording.2024-10-14.at.3.14.47.PM.mov
I may have spoken too soon - it appears that if you shatter an unassigned block, it is not replaced by unassigned children. We should decide whether we require assignment before splitting or not and address accordingly. My sense is that we would want users to be able to shatter and then assign

The shattering UDF has been updated, and this should be resolved.

raphaellaude · 2024-10-16T13:23:56Z

The shattering UDF has been updated, and this should be resolved.

To reviewers, cc @mariogiampieri, if you're testing locally you'll need to re-run migrations or use psql to redefine the backend/app/sql/shatter_parent.sql UDF.

Here's the fix in action:

shattering_unassigned.mp4

raphaellaude added 2 commits September 8, 2024 22:25

refactor out tiles for now

50dd897

update deps

shattering models

6d81538

alembic migration script

raphaellaude self-assigned this Sep 9, 2024

parent child procedure and management commands

d91b544

raphaellaude added enhancement New feature or request API frontend CLI For managing gerrydb views, tiles, etc. anything non-public roadmap layers Contextual map layers including the basemap labels Sep 9, 2024

clean up cli

e76a9e1

add command for creating districtrmap

bailliekova reviewed Sep 11, 2024

View reviewed changes

raphaellaude added 10 commits September 13, 2024 17:28

start testing cli

05ad503

show cli coverage too

507ab52

reorg args and allow for child layer to be null

98b87ea

materialize unioned gerrydb views

aeb6ae3

refactor districtmap to use only gerrydb table names rather than uuids

8cc7fc0

move helpers functions into app

fix tests

ae5397b

update procedures and udf api

b43b1e5

intial draft of shatter parent udf

fix silly issue

feb30c2

getting close

b6ca1d6

but still not swapping the parents and children in shatter functions correctly getting empty set of children inserted

fix by just forcing gerrydb table name in districtrmap to be unique

324266d

in the future may want to pass the districtrmap uuid to shatter map so that we don't need to enfore unique gerrydb table name

raphaellaude requested a review from bailliekova September 16, 2024 00:13

bailliekova reviewed Sep 18, 2024

View reviewed changes

nofurtherinformation and others added 3 commits September 19, 2024 16:14

Add event listener, context menu information in store

23ffe27

Draft Context Menu component

7e599df

update models to match

1e44fe1

nofurtherinformation and others added 8 commits October 9, 2024 17:25

Revert "add exit shatter view"

5467492

This reverts commit c90cc5e.

Revert "query features filtered to captive ids"

c5b39de

This reverts commit f6bd236.

Revert "change map fill opacity on shatter, helper"

a21a89e

This reverts commit b4495bd.

Revert "Add paintable IDs and bbox to state on shatter"

2dac210

This reverts commit 911ecbd.

update script

1bccead

handle non shattering maps

e830ef1

disable shattering if no child layer

9f01d14

optionals

5e9ef4d

nofurtherinformation added 4 commits October 10, 2024 08:49

ssr safety

03d365d

Clear state on create document

a1302cc

swap .union for new set

1bc972c

TS and eslint not updated to standard methods

missing brackets

7bbfa55

raphaellaude marked this pull request as ready for review October 14, 2024 12:53

make load data script better for future refactor

1c85564

mariogiampieri requested changes Oct 14, 2024

View reviewed changes

app/src/app/components/ContextMenu.tsx Outdated Show resolved Hide resolved

app/src/app/components/sidebar/GerryDBViewSelector.tsx Outdated Show resolved Hide resolved

bailliekova reviewed Oct 15, 2024

View reviewed changes

bailliekova previously approved these changes Oct 15, 2024

View reviewed changes

nofurtherinformation added 2 commits October 15, 2024 10:05

ruff

58a2e38

UDF Fix for shattering null parents

378da62

nofurtherinformation dismissed bailliekova’s stale review via 378da62 October 15, 2024 15:09

nofurtherinformation added 2 commits October 15, 2024 10:22

Swap onClick for onSelect

3ee9595

https://github.com/districtr/districtr-v2/pull/84/files/1c855645d4aac41494c3e5a33ea9b09294b3c2be#r1799832575

Bump limit to 30

a1c7de2

mariogiampieri approved these changes Oct 16, 2024

View reviewed changes

nofurtherinformation merged commit 1b1aa50 into main Oct 16, 2024

nofurtherinformation deleted the shattering branch October 16, 2024 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shattering #84

Shattering #84

raphaellaude commented Sep 9, 2024 •

edited

Loading

bailliekova Sep 11, 2024

raphaellaude commented Sep 16, 2024

bailliekova commented Sep 18, 2024

bailliekova left a comment

bailliekova Sep 18, 2024

raphaellaude Sep 19, 2024

bailliekova Sep 18, 2024

raphaellaude Sep 19, 2024

bailliekova Sep 18, 2024

raphaellaude Sep 19, 2024

raphaellaude commented Sep 19, 2024

raphaellaude commented Oct 10, 2024

nofurtherinformation commented Oct 14, 2024

mariogiampieri left a comment

mariogiampieri commented Oct 14, 2024

bailliekova Oct 15, 2024

bailliekova left a comment

bailliekova Oct 15, 2024

nofurtherinformation Oct 15, 2024

raphaellaude Oct 15, 2024

bailliekova Oct 15, 2024

raphaellaude Oct 15, 2024

raphaellaude Oct 15, 2024

nofurtherinformation commented Oct 15, 2024

raphaellaude commented Oct 16, 2024

Shattering #84

Shattering #84

Conversation

raphaellaude commented Sep 9, 2024 • edited Loading

Todo

Description

Reviewers

Checklist

Screenshots (if applicable):

Choose a reason for hiding this comment

raphaellaude commented Sep 16, 2024

bailliekova commented Sep 18, 2024

bailliekova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphaellaude commented Sep 19, 2024

raphaellaude commented Oct 10, 2024

nofurtherinformation commented Oct 14, 2024

mariogiampieri left a comment

Choose a reason for hiding this comment

mariogiampieri commented Oct 14, 2024

Choose a reason for hiding this comment

bailliekova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nofurtherinformation commented Oct 15, 2024

raphaellaude commented Oct 16, 2024

raphaellaude commented Sep 9, 2024 •

edited

Loading