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

Various RAWR tile fixes #281

Merged
merged 7 commits into from
Nov 3, 2017
Merged

Conversation

zerebubuth
Copy link
Member

A slightly random grab-bag of changes that were necessary to get RAWR tiles rendering in a not-awful way.

  • Cache the bounds of a LazyShape, and use that to skip unnecessary calls to geometry intersection. This doesn't save as much time as I'd have hoped, but it seems like a relatively benign bit of code to leave in.
  • Store sorted list of route names, rather than set. Some versions of ujson seem to be unhappy with serialising a set. I think the version we're using in prod is okay, but I managed to randomly update mine locally while playing around with different Shapely versions.
  • Handle missing nodes, ways and relations in the index. In an attempt at efficiency, I try to only index elements which are "interesting" and have a high likelihood of being looked up. However, at index-build time we don't have full information about relation membership, so there can be members of relations which I didn't deem "interesting". I think this is okay, and these elements wouldn't have contributed to the tile data.
  • Be consistent about line / linestring usage. Move it all to Enum-land instead.
  • Handle small numerical precision issues in conversion of bounding box to tile coverage. This was causing a slight (around 1e-6) change in coordinates to grab the data for the tile above, most of the contents of which were promptly discarded as they were outside the bounding box. Added a test for this too.

Matt Amos added 5 commits November 1, 2017 19:51
This can happen because we don't have perfect information about bi-directional linkages when building the index and, rather than index every element, the index is selective about what it stores. When the selection doesn't anticipate relation membership, it can result in a reference to a non-indexed element. At the moment, I don't think this is a problem, as those elements wouldn't have been "interesting" anyway.
…json seem to be unhappy with serialising a set.


def parse_shape_types(inputs):
lookup = {
Copy link
Member

Choose a reason for hiding this comment

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

Might be better to have this lookup stored somewhere outside the function so it doesn't have to get re-created for every call of parse_shape_types(). It might fit in as a @classmethod on ShapeType?

Copy link
Member

Choose a reason for hiding this comment

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

We might be able to be even more clever by leaning on enums.

  class ShapeType(Enum):
      point = 1
      line = 2
      polygon = 3
  
      multipoint = 1
      linestring = 2
      multilinestring = 2
      polygon = 3
      multipolygon = 3

Lookup is then just:

ShapeType[value.lower()]

which raises a KeyError if not found.

Although the code as it is should work, I don't think this is a good idea since it can lead to some confusing behavior:

>>> ShapeType.multipolygon
<ShapeType.polygon: 3>

Anyway, just mentioning this but I like @iandees more. We could even just stick the lookup at global scope if the interaction with sticking it on enums is awkward.



def parse_shape_types(inputs):
lookup = {
Copy link
Member

Choose a reason for hiding this comment

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

We might be able to be even more clever by leaning on enums.

  class ShapeType(Enum):
      point = 1
      line = 2
      polygon = 3
  
      multipoint = 1
      linestring = 2
      multilinestring = 2
      polygon = 3
      multipolygon = 3

Lookup is then just:

ShapeType[value.lower()]

which raises a KeyError if not found.

Although the code as it is should work, I don't think this is a good idea since it can lead to some confusing behavior:

>>> ShapeType.multipolygon
<ShapeType.polygon: 3>

Anyway, just mentioning this but I like @iandees more. We could even just stick the lookup at global scope if the interaction with sticking it on enums is awkward.

This means that the lookup dictionary isn't being created for each call, and that the code is "adjacent" to the Enum it depends on, which hopefully makes it easier to understand.
@zerebubuth zerebubuth force-pushed the zerebubuth/various-rawr-tile-fixes branch from ef38ef7 to 2cb637f Compare November 2, 2017 10:31
@zerebubuth
Copy link
Member Author

I ended up using the Enum aliases in 2cb637f because I couldn't figure out how to get a dictionary defined at class level to refer to the enumeration values, also defined at class level.

The alias method seems to work well, and I think it's quite readable. I put a comment in there to hopefully warn people away from using the aliases, and we'll have to see whether any other unexpected behaviour crops up as a result.

Please let me know what you think.

fetch=0,
intersect=0,
)
return [], metrics, timing
Copy link
Member

Choose a reason for hiding this comment

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

Sorry! :(

@rmarianski
Copy link
Member

The alias method seems to work well, and I think it's quite readable. I put a comment in there to hopefully warn people away from using the aliases, and we'll have to see whether any other unexpected behaviour crops up as a result.

Yea, this seems fine to me 👍 . I like that you put in a comment in there.

@zerebubuth
Copy link
Member Author

Thanks!

@zerebubuth zerebubuth merged commit 25bb3f2 into master Nov 3, 2017
@zerebubuth zerebubuth deleted the zerebubuth/various-rawr-tile-fixes branch November 3, 2017 16:21
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