Skip to content

Add collision rank#1793

Merged
zerebubuth merged 15 commits intomasterfrom
zerebubuth/988-add-collision-rank
Jan 18, 2019
Merged

Add collision rank#1793
zerebubuth merged 15 commits intomasterfrom
zerebubuth/988-add-collision-rank

Conversation

@zerebubuth
Copy link
Copy Markdown
Member

@zerebubuth zerebubuth commented Jan 16, 2019

Adds a method for assigning collision ranks to features.

Method

The idea of a collision rank is that it's a unique integer assigned to everything that gets labelled. See #988 for more background. This adds a new resource, spreadsheets/collision_rank.yaml, which is a YAML-encoded list of filters. The syntax of the filters is the same as the kind & min_zoom YAML in the yaml/ directory (hopefully this minimises the amount of duplication, and means we can share bug fixes and/or new features).

Each feature is checked against each filter in turn, and is assigned the collision_rank corresponding to the matching index. Indices start at 1, are incremented by one for each filter, and can have "reserved blocks" which skip the index counter onwards. For example, if collision_rank.yaml contains:

- kind: foo
- kind: bar
- _reserved: {from: 10, to: 20}
- kind: baz

Then a feature with:

  • kind: foo would get collision_rank: 1,
  • kind: bar would get collision_rank: 2,
  • kind: baz would get collision_rank: 21.

Note that indices 3-9 are unused in this example, but if we added an entry after bar, then it would be assigned 3. The code asserts that indices are unique, so we can't accidentally overflow the reserved blocks. Both the from and to values are inclusive, hence baz is assigned the next index, 21.

Why not use a CSV

We've been managing sort_rank with CSVs, but the issue is that we often don't care about the exact value of the sort_rank, but want to easily insert values and re-order the following lines. Having the sort_rank explicitly in the file means that any such re-arrangement has a very large diff, and is hard to check visually. This is an experiment to see if we can make something that still gives the features we want, but without the drawbacks of the CSV.

Additionally, the CSVs tended to get mangled by Excel when loading and/or saving, as we were using a bunch of special characters to indicate things such as "any" * or "must be null" - .

Finally, CSV files can't contain comments, but YAML ones can. This will make future maintenance easier, as we can comment # this reserved block for X, Y or Z.

Connects to #988.

@nvkelso
Copy link
Copy Markdown
Member

nvkelso commented Jan 16, 2019

👀

@rmarianski
Copy link
Copy Markdown
Member

LGTM so far!

@nvkelso
Copy link
Copy Markdown
Member

nvkelso commented Jan 17, 2019

@zerebubuth Please update the example config file based on the table in #988 (comment).

We should also add some tests based on a handful of resulting collision_rank values?

@zerebubuth
Copy link
Copy Markdown
Member Author

Updated the config in 961030e and added tests in eec237e. I think this is ready for a proper review!

@zerebubuth zerebubuth changed the title WIP: Add collision rank Add collision rank Jan 17, 2019
Comment thread spreadsheets/collision_rank.yaml Outdated
Comment thread spreadsheets/collision_rank.yaml Outdated
Comment thread spreadsheets/collision_rank.yaml Outdated
Comment thread spreadsheets/collision_rank.yaml Outdated
Comment thread spreadsheets/collision_rank.yaml Outdated
Comment thread spreadsheets/collision_rank.yaml Outdated
Comment thread spreadsheets/collision_rank.yaml Outdated
Comment thread spreadsheets/collision_rank.yaml Outdated
Comment thread spreadsheets/collision_rank.yaml Outdated
Comment thread spreadsheets/collision_rank.yaml Outdated
Comment thread spreadsheets/collision_rank.yaml Outdated
Comment thread spreadsheets/collision_rank.yaml Outdated
Comment thread spreadsheets/collision_rank.yaml Outdated
Comment thread spreadsheets/collision_rank.yaml Outdated
Comment thread queries.yaml
Copy link
Copy Markdown
Member

@nvkelso nvkelso left a comment

Choose a reason for hiding this comment

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

This looks great!

See comments for a few requested changes. They don't substantively change the logic and are mostly focused on the data config file, but should be changed before merging.

…hing on the tile layer the feature is in. This avoids a clash with the layer property, which is about feature z-ordering.
…r in previous gap calculation. Align ranges to round numbers. Fix tests to match.
…ref or something else that would be labelled in the properties. Extend country test to check for name:left and name:right.
Copy link
Copy Markdown
Member

@nvkelso nvkelso left a comment

Choose a reason for hiding this comment

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

Most excellent!

@zerebubuth zerebubuth merged commit b28a7ea into master Jan 18, 2019
@zerebubuth zerebubuth deleted the zerebubuth/988-add-collision-rank branch January 18, 2019 17:24
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.

3 participants