Skip to content

fix superseded parent IDs - #23

Open
missinglink wants to merge 4 commits into
whosonfirst-data:masterfrom
missinglink:fix-superseded-parent-ids
Open

fix superseded parent IDs#23
missinglink wants to merge 4 commits into
whosonfirst-data:masterfrom
missinglink:fix-superseded-parent-ids

Conversation

@missinglink

@missinglink missinglink commented Jul 8, 2020

Copy link
Copy Markdown

This PR fixes the superseded parent IDs as discussed in whosonfirst-data/whosonfirst-data#1377


I produced these changes with the following commands:

# clone repo (fa984290faaef47d83c795159558cf7ee5560ca4)
git clone https://github.com/whosonfirst-data/whosonfirst-data-admin-ca.git

# convert to sqlite db
wof git export whosonfirst-data-admin-ca | wof sqlite import ca-fix.db

# fix IDs, print updated records to stdout, exportify them and write them back to the filesystem
wof sqlite fix --hierarchies --dryrun --cat ca-fix.db \
  | wof -v fs import --exportify whosonfirst-data-admin-ca/data

The actual 'fix' code itself lives in https://github.com/pelias/wof/blob/master/sqlite/fix.js
A brief summary of how the wof sqlite fix command works:

  1. Find all records with wof:supersedes set to a non-empty array
  2. Create a map where the key is the superseded ID and the value is the replacement ID
  3. Get a list of record IDs which reference these superseded IDs in their hierarchy AND are also not mz:is_current=0
  4. Fetch those records (again skipping any non-current records) and patch them:
    4.1 Update wof:parent_id if in replacement map
    4.2 Update hierarchies, any self-references are skipped

I can go ahead and open PRs for all the other repos too, but using CA first as a testcase.
Once we discuss/merge this I'll use the same process for the others which should make the review/merge process easier.

I'm pretty comfortable with this workflow, so hopefully it's only the first of many machine generated PRs from me 🤖

@missinglink

missinglink commented Jul 8, 2020

Copy link
Copy Markdown
Author

I spotted an error in https://github.com/whosonfirst-data/whosonfirst-data-admin-ca/pull/23/files#diff-322a3e7cd45c1904f9bca3e39dae55f9

Screenshot 2020-07-08 at 10 02 21

Screenshot 2020-07-08 at 10 02 15

In that case it seems as though the locality was superseded by a county, I hadn't accounted for this case..

@missinglink

missinglink commented Jul 8, 2020

Copy link
Copy Markdown
Author

Okay, I fixed the bug mentioned above with pelias/wof#21 and added e3bb499.

Let me know if you'd like to to squash the commits before merge? I thought it's nicer to have two for the review 😄
[edit] we should definitely squash it or the history will be weird

@missinglink

Copy link
Copy Markdown
Author

Caught one more (fairly rare) bug, fixed in pelias/wof#22, added 505cc0e

"mz:hierarchy_label":1,
"mz:is_current":1,
"mz:min_zoom":13.0,
"mz:min_zoom":13,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our zoom values should remain floats (13.0, rather than 13).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs were run through 'exportify' so I don't have any control over how they are serialised 🤷

"continent_id":102191575,
"country_id":85633041,
"county_id":890456343,
"locality_id":101740755,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in this case, the record currently has a dual hierarchy, but only one of the hierarchies is valid.. we should remove this entire hierarchy set so this feature only has one hierarchy (the one below, with "locality_id":101735893).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I detect an valid/invalid hierarchy?

@missinglink missinglink Jul 8, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a56eabe which I think is the correct fix.

// deduplicate hierarchies
// remove any hierarchies which are duplicates of, or a subset of another hierarchy
const deduplicatedHierarchies = hierarchies.filter((h1, b1) => {
  return !hierarchies.some((h2, b2) => (b1 != b2) && _.isMatch(h2, h1))
})

[edit] this deduplication algo was improved further and tested in pelias/wof#25

"lbl:latitude":43.676352,
"lbl:longitude":-79.628342,
"lbl:max_zoom":18.0,
"lbl:max_zoom":18,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same goes for lbl zooms, let's keep them float values (18.0, rather than 18).

"edtf:inception":"uuuu",
"geom:area":0.0,
"geom:area_square_m":0.0,
"geom:area_square_m":0,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm... exportify tool?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the command I'm using to run exportify in docker:
https://github.com/pelias/wof/blob/master/whosonfirst/exportify.js

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confuddled by this, why was geom:area_square_m converted to 0 but the line above geom:area remained 0.0 😕

@missinglink

Copy link
Copy Markdown
Author

opened an issue over at exportify whosonfirst/py-mapzen-whosonfirst-export#28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants