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

More kinds for maki icon library support #1541

Merged

Conversation

zerebubuth
Copy link
Member

Added some more kinds related to #1423.

  • gaming
  • garden_centre
  • golf
  • grocery
  • harbor
  • naval_base
  • port_terminal
  • ferry_terminal landuse
  • container_terminal
  • shipyard
  • wharf
  • quay
  • boatyard
  • boat_lift
  • ship_chandler
  • customs

(NOTE: we already had man_made=crane mapped to kind: crane, so there wasn't a need to add that.)

Some follow-ups:

  • Added a kind for grocery. We already had greengrocer and, looking at the Wikipedia pages for grocery compared with greengrocer, I'm not sure that they're the same thing. For the moment, I've left them as separate kinds.
  • Many items tagged as harbour=yes are also tagged as other things. For example, this port terminal is tagged harbour=yes, landuse=port_terminal, man_made=pier; should that be output as a harbour, port terminal or pier - or should we split the geometry to allow multiple features for the harbour landuse and pier, with a POI for the port terminal? Also, this pier is tagged harbour=yes, man_made=pier - should that be harbour, pier or split into two features?
  • The customs kind mentions the amenity=customs tag, which has only 742 uses. Should we also include the barrier=border_control tag, which has over 5,000 uses? Should they both be mapped to kind customs, or are customs (i.e: taxation) and border control sufficiently distinct things to deserve distinct kinds?

@zerebubuth zerebubuth requested a review from nvkelso June 20, 2018 16:42
@nvkelso
Copy link
Member

nvkelso commented Jun 20, 2018

  • Added a kind for grocery. We already had greengrocer and, looking at the Wikipedia pages for grocery compared with greengrocer, I'm not sure that they're the same thing. For the moment, I've left them as separate kinds.

In SF they are different things, I think we should set as different kind values. Not saying they're always tagged right, but they are different.

  • Many items tagged as harbour=yes are also tagged as other things. For example, this port terminal is tagged harbour=yes, landuse=port_terminal, man_made=pier; should that be output as a harbour, port terminal or pier - or should we split the geometry to allow multiple features for the harbour landuse and pier, with a POI for the port terminal? Also, this pier is tagged harbour=yes, man_made=pier - should that be harbour, pier or split into two features?

Seems like harbour=yes is being used as a generic grouper here that we should ignore and we should only use and export landuse%3Dharbour.

For 256125920 I'd expect port_terminal. Not excited about splitting for multiple features unless there's a compelling case for it.

For 183049783 I'd expect pier.

  • The customs kind mentions the amenity=customs tag, which has only 742 uses. Should we also include the barrier=border_control tag, which has over 5,000 uses? Should they both be mapped to kind customs, or are customs (i.e: taxation) and border control sufficiently distinct things to deserve distinct kinds?

Both as distinct kinds, please.

BTW, barrier=border_control is requested in #1424 which is in this milestone, can include in this PR or in later PRs.

min_zoom: 16
output:
<<: *output_properties
kind: harbor
Copy link
Member

Choose a reason for hiding this comment

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

Our practice on the kinds is generally to follow British English, especially if that's what the original OSM tag was.

Please set this to harbour.

Copy link
Member

@nvkelso nvkelso Jun 20, 2018

Choose a reason for hiding this comment

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

See PR comments – this should move to landuse: harbour only, and can be merged with the next filter.

Copy link
Member Author

Choose a reason for hiding this comment

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

harbor -> harbour in 96110df.

@@ -1614,6 +1614,13 @@ filters:
output:
<<: *output_properties
kind: petroleum_well
# a boat lift is a more specific type of object than a crane, although it may
Copy link
Member

Choose a reason for hiding this comment

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

👍 Useful note explaining the order of filters.

yaml/pois.yaml Outdated
min_zoom: 17
output:
<<: *output_properties
kind: gaming
Copy link
Member

Choose a reason for hiding this comment

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

I think the kind should be adult_gaming_centre

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed in 96110df.

output:
<<: *output_properties
kind: naval_base
kind_detail: military
Copy link
Member

Choose a reason for hiding this comment

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

Why is the kind_detail set here? Are there naval bases that aren't military related?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, sorry, I think that was copy-pasta from the above airfield, where it makes sense to say it's a military thing. Civilian airfields exist, but civilian naval bases, as you rightly point out, not so much. Fixed in 96110df.

self.assert_has_feature(
z, x, y, 'landuse', {
'id': 183049783,
# should be 'kind': u'harbor',?
Copy link
Member

Choose a reason for hiding this comment

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

Resolve this before merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved in 96110df.

docs/layers.md Outdated
@@ -524,6 +526,7 @@ _TIP: Some `landuse` features only exist as point features in OpenStreetMap. Fin
* `groyne`
* `guard_rail`
* `hanami`
* `harbor` (a.k.a harbour)
Copy link
Member

Choose a reason for hiding this comment

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

Also fix the documentation spelling (to harbour without the parenthetical note)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooops, well spotted. Thanks. Fixed in 745f2e5.

docs/layers.md Outdated
@@ -879,16 +890,20 @@ To resolve inconsistency in data tagging in OpenStreetMap we normalize several o
* `fuel` - Fuel stations provide liquid gas (or diesel) for automotive use.
* `furniture`
* `gallery` - An art gallery.
* `gaming`
Copy link
Member

@nvkelso nvkelso Jun 20, 2018

Choose a reason for hiding this comment

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

This name also changes to adult_gaming_centre?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. Fixed in 745f2e5.

docs/layers.md Outdated
* `golf_course`
* `government`
* `greengrocer`
* `greengrocer` - Shop selling fruits and vegetables.
* `grocery` - Shop selling non-perishable foods.
Copy link
Member

Choose a reason for hiding this comment

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

Shop selling non-perishable foods, also known as a "supermarket".?

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 changed it to:

Shop selling non-perishable food often similar to, but smaller than, a supermarket. See also grocery store on Wikipedia.

I get the impression from that Wikipedia article that a grocery store doesn't generally sell fresh fruits and vegetables, nor non-food items. I can't remember ever having been in a store which had such a laser focus - all the ones which come to mind either additionally sold stationery, cards, household products and similar non-food items (and might be called a newsagents or convenience store), or had a fridge/freezer section for selling perishable food.

Should we merge this into supermarket or convenience?

Copy link
Member

Choose a reason for hiding this comment

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

In the USA grocery is probably more akin to "corner store" or "bodega" versus a supermarket which is a usually "big box" style store. Fine as-is from OSM.

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 asside from couple minor documentation change requests.

@@ -1116,3 +1132,14 @@ filters:
<<: *output_properties
kind: crane
kind_detail: {col: "crane:type"}
- filter:
landuse:
- harbour
Copy link
Member

Choose a reason for hiding this comment

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

I think harbour should go into the general port group above, with the min zoom clamp of 11 to 16 ... Possibly also port.

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. Moved harbour and port in 745f2e5.

- shipyard
- wharf
- quay
min_zoom: 16
Copy link
Member

Choose a reason for hiding this comment

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

15 for port and shipyard, 16 for wharf and quay?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, and added tests in 745f2e5.

…nd gaming->adult_gaming_centre. Altered min_zooms for port or harbour-related landuse.
@zerebubuth zerebubuth merged commit 19f65a8 into master Jun 21, 2018
@zerebubuth zerebubuth deleted the zerebubuth/1423-kinds-for-maki-icon-library-support branch June 21, 2018 11:34
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.

2 participants