-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new US counties in CT, AK, and VA and rework related CT localadmin #207
base: master
Are you sure you want to change the base?
Conversation
@stepps00 The commits in this PR should be fairly atomic and reviewable in sequence and in total. For the counties I manually set the wof:hierarchy & etc, but really it needs a fuller PIP to account for all the child places in these counties (not just localadmin but also locality and neighbourhoods, etc). |
I'll followup with a separate PR for the localadmin work outside of Connecticut as that affects many thousand places, but doesn't require PIP. |
Followup outside of these new features in CT and AK for statistical gores is in #208. |
data/102/084/283/102084283.geojson
Outdated
@@ -2,7 +2,7 @@ | |||
"id": 102084283, | |||
"type": "Feature", | |||
"properties": { | |||
"edtf:cessation":"uuuu", | |||
"edtf:cessation":"2021-12-31", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and a few others should get an update to the wof:lastmodified
date field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by re-exportifying everything!
Looks good - a few comments, mostly around missing or unexpected properties. Once these features get Exportified and PIPd, I can take a second look before merge. |
@stepps00 I've addressed your PR review comments and re-exportified everything. One more 👀 ? Then I'll pick this up later for final PIP round. |
@@ -242,7 +242,7 @@ | |||
"wof:lang": [ | |||
"eng" | |||
], | |||
"wof:lastmodified": 1722546126, | |||
"wof:lastmodified": 1722625362, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. looks like this feature hasn't been updated, aside from this property?
@@ -484,7 +484,7 @@ | |||
} | |||
], | |||
"wof:id": 1125864515, | |||
"wof:lastmodified": 1722545441, | |||
"wof:lastmodified": 1722625363, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue, this feature only has the lastmodified date edited.
A few records only have lastmodified date updates.. aside from that, LGTM. I can take another look once the PIP work is added. |
@stepps00 Any updates? |
The next step here is to PIP update the hierarchy and parent attributes. I haven't had time to do that yet... |
@nvkelso just curious what do you mean by PIP update? |
PIP means "point in polygon" and is PIPing is the process of taking the revgeo centroid, or the label centroid, or the math centroid and doing a spatial join with all the possible parent polygons to then update the We used to have a semi-automated workflow for PIPing, but we're more low fi now. @thisisaaronland described how this can be accomplished using modern workflow in blog post, but I haven't found time to get that up and running on my new dev machine. There's some WOF specific business logic for the hierarchy calculations... else you could manually run the PIP in QGIS with a and with a little bit of scripting construct the new message and update each record. |
Fixes whosonfirst-data/whosonfirst-data#2197 (CT) and whosonfirst-data/whosonfirst-data#2136 (AK and VA).
Needs full PIP