-
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
Tile size: water only changes #2010
Conversation
params: | ||
source_layer: water | ||
start_zoom: 0 | ||
end_zoom: 16 |
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.
exclusive
c99cca3
to
405361c
Compare
This PR has 95 integration tests failures and 23 errors vs master where it has 92 failures and 0 errors 23 errors vs master:
3 new Failures vs master:
|
made a commit to try to fix the integration tests errors (not failures) |
# should only have top 10 features | ||
self.assertEqual(10, len(features)) | ||
# should only have top 2 features | ||
self.assertEqual(2, len(features)) |
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.
At zoom 8 our area threshold for labels is 500,000,000 (was 200,000,000) and for polygons it's still 20000000.
Under the earlier system 5 meet the area threshold, but after only 2 do. So the changes here make sense to me.
end_zoom: 15 | ||
properties: | ||
- id | ||
- area |
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.
could add boundary
and osm_relation
, too
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.
Addressed via 77e663c.
queries.yaml
Outdated
- fn: vectordatasource.transform.drop_properties | ||
params: | ||
source_layer: water | ||
start_zoom: 8 |
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 should be for low zoom tiles as well, so 8
> 0
.
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.
Addressed via 77e663c.
queries.yaml
Outdated
- fn: vectordatasource.transform.drop_properties | ||
params: | ||
source_layer: water | ||
start_zoom: 8 |
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.
Low zoom too, so 8
> 0
.
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.
Addressed via 77e663c.
queries.yaml
Outdated
] if area >= area_threshold][0] | ||
where: >- | ||
kind == 'lake' and label_placement | ||
kind in ['lake','bay','water','riverbank','reservoir'] and label_placement |
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.
I'm seeing basin
and dock
label points very early still in London, and strait
along the English Channel.
The full list of water kinds is:
basin, bay, canal, ditch, dock, drain, fjord, fountain, lake, playa, reef, river, riverbank, strait, stream, swimming_pool, water
But perhaps it's too brittle to have a full list (since values will come and go), when the measure should just be that we generated a label_placement for it at all?
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.
Perhaps:
where: >-
(properties is not None and properties.get('label_placement') is 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.
Addressed via 77e663c.
queries.yaml
Outdated
- wikidata_id | ||
- kind_tile_rank | ||
- label_placement | ||
where: >- |
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.
Also drop boundary
which randomly appears when a river is also forms part of an administrative boundary.
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 where clause doesn't seem to work.
There's another pattern we can switch to, like:
# no lake labels at zoom 0-3:
# https://github.com/tilezen/vector-datasource/issues/1730
- fn: vectordatasource.transform.drop_names
params:
source_layer: water
start_zoom: 0
end_zoom: 4
geom_types: [Polygon, MultiPolygon]
where: >-
kind == 'lake'
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.
Addressed via 77e663c.
(6, 40000000000), | ||
(7, 10000000000), | ||
(8, 500000000), | ||
(9, 200000000), |
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.
The larger range of min_zoom
remappings is working for features are in the allowlist, woot!
queries.yaml
Outdated
all_name_variants: true | ||
properties: | ||
- name | ||
where: >- |
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.
Also: old_name
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.
Addressed via 77e663c.
queries.yaml
Outdated
properties: | ||
- name | ||
where: >- | ||
geom_type in ('Polygon', 'MultiPolygon') |
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.
Same where clause switch as above.
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.
Addressed via 77e663c.
self.assert_has_feature( | ||
z, x, y, 'water', { | ||
'kind': 'lake', | ||
'label_placement': True, | ||
'min_zoom': 5, | ||
'min_zoom': 2, # min_zoom changed at https://github.com/tilezen/vector-datasource/pull/2010/ |
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.
Only the 'label_placement': True
point should have names at this early zoom, and the min_zoom
on that should probably be a different value besides 2 (like 7)?
source_layer: water | ||
start_zoom: 0 | ||
end_zoom: 15 | ||
geom_types: [LineString, MultiLineString, Polygon, MultiPolygon] |
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.
Switched to geom_types param instead of where
clause.
source_layer: water | ||
start_zoom: 0 | ||
end_zoom: 15 | ||
geom_types: [Polygon, MultiPolygon] |
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.
Switched to geom_types param instead of where
clause.
(kind == 'water' and zoom < 12) or | ||
(kind == 'canal' and zoom < 13) or | ||
(kind == 'basin' and zoom < 13) or | ||
(kind == 'dock' and zoom < 13) or |
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.
I expanded the list of kinds in this section based on QA where I saw low zoom basin
, dock
and straight
points around London and the English Channel. Now it has every water kind.
Final QA over Sweden checks out, all remaining issues resolved. Ready to merge. |
two new failures:
|
Those look like they should be updated, but don't show a problem with the new logic. Let's keep tracking that with #2038, but merge this now. |
Connects to #1998 and replaces PR #2000 (RIP).