Skip to content
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

Alternate viewpoints (v0.1) #1895

Merged
merged 14 commits into from
May 15, 2019
Merged

Conversation

zerebubuth
Copy link
Member

Added support for alternate viewpoints of country borders.

Alternate viewpoints are used where two or more countries have different opinions about where their borders are. In many cases, the governments of these countries require that maps display their particular viewpoint, so it's very practically useful to have data in the tiles to support that.

The data output in the tiles will have the usual kind for de-facto boundaries, but additional kind:xx data for the viewpoint of country XX (ISO 3166-1 alpha-2 code). For generally-recognised boundaries that XX thinks are interior to its territory, we set kind = country, kind:xx = unrecognized_country. For generally not recognised boundaries that XX thinks are its borders, we set kind = unrecognized_country and kind:xx = country. Therefore, it's possible to recover the status of any given border by a similar method we use for translations; use kind:yy if it exists for the viewpoint YY, falling back to the regular kind if not.

This is done by looking at:

  1. Claim relations. These give us the extra geometry which is generally unrecognised, but a country boundary for the viewpoint in the claimed_by tag or other viewpoints listed in the recognized_by tag. (NOTE: recognized_by support is still a TODO).
  2. Disputed ways. Ways tagged dispute(d)=yes and with a list of viewpoints in disputed_by will "erase" those borders for the listed viewpoints. This is done by intersecting a buffered version of the ways with the polygonally-derived country borders.

Until now, we had only been including boundary features from the planet_osm_polygon table and nothing at all from the planet_osm_line table. However, the disputes and claims are linear features, so we needed to include those.

However, because osm2pgsql creates both linear and polygonal features for boundaries (ways and relations), then we must exclude all the linear features that are in planet_osm_line except for the disputes and claims. This is further complicated because we change the geometry type during the query, dropping the polygon for its (oriented) boundary.

The way I've done this is to include a new flag, mz_boundary_from_polygon, which basically means "this was a polygon, but we transformed it", and this allows us to write YAML that works. However, it's kind of a horrible hack and I mildly regret doing it. A better alternative might have been to leave the polygons in the query (appropriately clipped to an expanded bounding box) and do the boundary transform at a later stage in the stack - however this might be left for a rainy day issue.

Things to note:

  • The disputed ways must be part of the generally-recognised country borders. It's not possible to dispute a claim - nor is it necessary, since claims are only "visible" to the claimed_by and recognized_by viewpoints.
  • Neither disputes nor claims go through the left/right matching process, so the name property on them is whatever the name tag was on the claim relation or disputed way.
  • Since we don't build the claimed polygon, we aren't able to figure out what other features (internal borders, locality / place points, etc...) are in the claimed area, which limits our ability to automatically hide or adjust the kind of features in that area according to the claimant viewpoint.

Still TODO:

  • Other admin boundaries (region, county, etc...)
  • Support for recognized_by.

Connects to #1810.

zerebubuth added 2 commits May 9, 2019 11:29
Until now, we had only been including boundary features from the `planet_osm_polygon` table and nothing at all from the `planet_osm_line` table. However, the disputes and claims are linear features, so we needed to include those.

However, because `osm2pgsql` creates both linear and polygonal features for boundaries (ways and relations), then we must exclude all the linear features that are in `planet_osm_line` _except_ for the disputes and claims. This is further complicated because we change the geometry type during the query, dropping the polygon for its (oriented) boundary.

The way I've done this is to include a new flag, `mz_boundary_from_polygon`, which basically means "this was a polygon, but we transformed it", and this allows us to write YAML that works. However, it's kind of a horrible hack and I mildly regret doing it. A better alternative might have been to leave the polygons in the query (appropriately clipped to an expanded bounding box) and do the boundary transform at a later stage in the stack.

Lots of changes to the integration tests to adjust for the new hacky flag...
@zerebubuth zerebubuth requested review from rmarianski and nvkelso May 13, 2019 16:08
zerebubuth added a commit to tilezen/tilequeue that referenced this pull request May 13, 2019
In tilezen/vector-datasource#1895, I added a little hack to work around the way we change the geometry type of `boundaries` layer features. They used to come entirely from the `planet_osm_polygon` table, but now we also include disputes and claims from `planet_osm_line`. However, we have to filter out a bunch of features from `planet_osm_line` that we don't want, but have the same tags as the polygons we do want.

Long story short: The query in `vector-datasource` sets this flag when transforming a polygon into a boundary line, so we have to do the same thing when reading from the RAWR tiles.
@nvkelso
Copy link
Member

nvkelso commented May 13, 2019

Nice! I see mz_boundary_from_polygon in a lot of the tests. Is that to make sure things are wired up for QA and then we'd drop that property before cutting the release?

'kind': 'unrecognized_country',
'kind:bb': 'country',
'kind:cc': 'country',
# 'kind:dd': 'country',
Copy link
Member

Choose a reason for hiding this comment

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

TODO: This will be uncommented out once recognized_by is plumbed thru.


self.assert_has_feature(
z, x, y, 'boundaries', {
'kind': 'unrecognized_country',
Copy link
Member

@nvkelso nvkelso May 13, 2019

Choose a reason for hiding this comment

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

I wonder, if like the name fan out we did in #1454, if we should also indicate kind:aa and kind:dd = unrecognized_claim, since it's meaningful those countries have an opinion (versus generic default)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the situation is a little bit different for boundaries. For the names, we were removing data that had come as input, which broke multi-level fallback. Single-level fallback worked just fine.

Is a situation in which we'd need to do multi-level fallback? In this test example, would we ever do a fallback sequence kind:aa, kind:bb, kind?

The disputed_by = AA;DD on the claim means that AA and DD dispute the claim, not whether they do or do not agree with the locations of the borders on the ground - I've assumed any viewpoint not listed in claimed_by or recognized_by doesn't agree (and will default to kind: unrecognized_*).

When I first prototyped this, projecting the disputed_by from relations onto constituent ways produced borders which were disputed_by pretty much everyone and disappeared from the map. So I'd be very wary of using the disputed_by information from claim relations.

Is there a real-world example of where it's useful to have a multi-level fallback projected from the claim relation?

Copy link
Member

Choose a reason for hiding this comment

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

Is a situation in which we'd need to do multi-level fallback?

Generally countries are "aligned" with some another country in that they have a few opinions about their immediate neighbours, but lean on either ISO or the aligned country's foreign policy.

In practice someone developing in country X shouldn't need to know if Tilezen does or doesn't have data for that country's worldview (POV), but they should still indicate that as the primary POV, and ideally list out ISO or the other country Y (which we likely have in the data) as fallback, before falling back to defacto kind values.

In this test example, would we ever do a fallback sequence kind:aa, kind:bb, kind?

I think it's more likely it'd be kind:xx (primary), kind:aa (secondary)

To be clear, I think the fallback only happens client side not server side.

I wonder, if like the name fan out we did in #1454, if we should also indicate kind:aa and kind:dd = unrecognized_claim, since it's meaningful those countries have an opinion (versus generic default)?

Is that possible in current data structure, or would it cause problems like you described? I'm open to not doing the fan out.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, cool. I added a test for this in 74b4b7f, although the logic is getting a little bit complicated now!

'https://www.openstreetmap.org/relation/148838',
], clip=self.tile_bbox(16, 10417, 25370, padding=2))
self.generate_fixtures(
# https://www.openstreetmap.org/relation/148838

This comment was marked as resolved.

#
# NOTE: these come _first_ so that they match in preference to regular
# country borders. (this assumes disputed_by is never set on a whole
# country boundary -- perhaps true?)
Copy link
Member

@nvkelso nvkelso May 13, 2019

Choose a reason for hiding this comment

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

These 2 are technically entirely disputed_by their neighbours, but instead tagged with claimed_by? How should this be setup / changed (data or processing)?

Taiwan: https://www.openstreetmap.org/relation/449220
Western Sahara: https://www.openstreetmap.org/relation/5441968

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I was mistaken when I wrote that comment. I've added a test for this in c2efb38, and it works, so I removed the comment and replaced it with better tagging recommendations.

For Western Sahara, the tagging looks good to me. I think 🤞 that it should work the next time we do a build.

For Taiwan, it's missing:

  • A boundary=claim relation with claimed_by=CN and admin_level=2. I had a look around, and couldn't find one. It should contain the same ways as relation 449220, the country border.
  • The ways making up the country border need disputed=yes and disputed_by=CN on them. I didn't audit all the ways, but a few tested at random (e.g: this one) were missing those tags.

Copy link
Member

Choose a reason for hiding this comment

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

Agree I need to add OSM tagging for Taiwan.

@nvkelso
Copy link
Member

nvkelso commented May 13, 2019

Great progress, this approach seems workable.

I left comments above, will review again once Other admin boundaries (region, county, etc...) and Support for recognized_by are added.

@zerebubuth
Copy link
Member Author

I see mz_boundary_from_polygon in a lot of the tests. Is that to make sure things are wired up for QA and then we'd drop that property before cutting the release?

No, it's an internal flag that's used to indicate a feature is the boundary line of an admin polygon, rather than from the lines table. I mentioned it in the PR description:

The way I've done this is to include a new flag, mz_boundary_from_polygon, which basically means "this was a polygon, but we transformed it", and this allows us to write YAML that works. However, it's kind of a horrible hack and I mildly regret doing it.

Note that we drop all mz_* properties from features before formatting the tile, so this flag never appears in the output.

@nvkelso
Copy link
Member

nvkelso commented May 14, 2019

I'd forgotten about the general drop all mz_* properties logic, makes sense now.

Copy link
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.

LGTM so far. There's a stub for _BOUNDARY_KINDS for more than country in this PR, we can continue more work in additional PRs to make it easier to review?

@zerebubuth
Copy link
Member Author

we can continue more work in additional PRs to make it easier to review?

Yes, sounds like a good idea!

@zerebubuth zerebubuth merged commit 425febb into master May 15, 2019
@zerebubuth zerebubuth deleted the zerebubuth/1810-alternate-viewpoints branch May 15, 2019 10:07
@zerebubuth zerebubuth restored the zerebubuth/1810-alternate-viewpoints branch June 12, 2019 10:39
@zerebubuth zerebubuth deleted the zerebubuth/1810-alternate-viewpoints branch June 12, 2019 13:19
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