-
Notifications
You must be signed in to change notification settings - Fork 119
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
roads layer thining z14 to z15 #2056
Conversation
peitili
commented
Feb 8, 2022
•
edited
Loading
edited
- Update tests
- Update docs
for more information, see https://pre-commit.ci
b327c54
to
8a825e1
Compare
z, x, y = tile | ||
self.assert_has_feature( | ||
z, x, y, 'roads', | ||
{'id': type(None), 'shield_text': 'A151'}) |
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.
@nvkelso basically I copied the implementation of _check_network_relation
and the only diffrence I made here is to assert 'id': type(None)
instead. Before it asserts the id == way_id
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.
LGTM
@@ -13,7 +13,7 @@ def test_motorway_bridge(self): | |||
self.assert_has_feature( | |||
16, 10472, 25323, 'roads', | |||
{'kind': 'highway', 'kind_detail': 'motorway', 'id': 28412298, | |||
'name': 'Presidio Pkwy.', 'is_bridge': True, 'sort_rank': 443}) | |||
'name': type(None), 'is_bridge': True, 'sort_rank': 443}) |
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 is because the feature is too short to fit the label, while at zoom 17 it would fit / we don't drop.
@@ -50,7 +50,7 @@ def test_secondary_level_0(self): | |||
self.assert_has_feature( | |||
16, 16812, 24391, 'roads', |
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.
If you updated this to 17, 33624, 48782
then the name would still be there, but it's fine this way.
Estimated impact on 512px sized tiles:
|
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.
LGTM
There are a few test failures that this PR and the last road layer thinning PR introduced. Let's tackle them in a followup PR.
@@ -114,9 +115,10 @@ def test_inbound_and_outbound_routes(self): | |||
16, 10477, 25327, 'roads', | |||
{'id': 225516711, | |||
'bus_network': type(None), | |||
'bus_shield_text': '3', | |||
'bus_shield_text': type(None), | |||
'is_bus_related': True, |
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 is wrong, the integ test says it should be None