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

Remove name from UK Post Office entry #4384

Merged
merged 3 commits into from Oct 9, 2020
Merged

Remove name from UK Post Office entry #4384

merged 3 commits into from Oct 9, 2020

Conversation

Cj-Malone
Copy link
Collaborator

Each UK Post Office has a unique name, the NSI shouldn't try to push people towards a inaccurate alternative.

It may be worth looking at other regions to see if the same should be done there.

@Adamant36
Copy link
Collaborator

Each UK Post Office has a unique name

Technically that's wrong. A lot are just called "Post Office." At least that's what is on the store signs. Which is what OSM usually goes with. Do a Google Image search for UK Post Office and you'll see a ton of them. For the few that are locally known as something different the name field can be edited pretty easily anyway. That's how it is with things like hotels that have regional variations in the name sometimes, but not always or that much. It works out fine that way.

@peternewman
Copy link
Collaborator

peternewman commented Oct 8, 2020

A lot of the ones I've seen look sort of like this:
https://www.alamy.com/stock-photo-the-post-office-stratford-upon-avon-warwickshire-uk-75283848.html

I'd therefore assumed the name for this is "Stratford-Upon-Avon Post Office", or is that not correct?

I guess it's one for another project, but if we want the brand stuff, we're in a bit of a catch 22 here, as we want it to match "Foo Post Office" to then suggest the brand/operator stuff, as it's unlikely to be populated otherwise unless it's suggested isn't it.

I think at a minimum that should become a matchNames shouldn't it?

@Cj-Malone
Copy link
Collaborator Author

OK, yeah, sometimes the fascias just say "Post Office". But quite often the fascia will say the official name (see the photo on wikidata), and as far as I understand it NSI is only for universal tags.

@peternewman Yeah, in OSM it's "Stratford Upon Avon Post Office", and the official Post Office Ltd name is "Stratford-upon-Avon Post Office".

I don't know how matchNames works but I'll look at adding it to this PR.

@Adamant36
Copy link
Collaborator

Adamant36 commented Oct 8, 2020

I don't think just "Post Office" is wrong in that case because the sign still says "Post Office" in the normal circle logo off to the side.

I guess a possible example would be something like image below, but I still don't think its "wrong" in that instance either. Since the location is a prefix and not technically the brand name. I don't think anyone viewing the map would be confused if it just said "Post Office" either. Also, I'm not sure how a match name or match tag would be helpful.
3898

@Adamant36 Adamant36 added check local This issue should be checked with the local mapping community for guidance considering Not Actionable - still considering if this is something we want and removed check local This issue should be checked with the local mapping community for guidance labels Oct 8, 2020
@peternewman
Copy link
Collaborator

as far as I understand it NSI is only for universal tags.

NSI can match on the brand or operator too/as well. So if you drop the name field, it could then notice that e.g. the brand matched, I think technically it having operator=Royal Mail may be wrong as Rob's site suggests its a franchise. I'm unclear if matchNames will work against the name field if one isn't included in the entry. @bhousel ?

@peternewman Yeah, in OSM it's "Stratford Upon Avon Post Office",

Isn't that wrong because it doesn't have the hyphens it did in the real world a few months earlier? Assuming they hadn't rebranded.

and the official Post Office Ltd name is "Stratford-upon-Avon Post Office".

But I see the Post Office itself isn't consistent on case!

I don't know how matchNames works but I'll look at adding it to this PR.

That's fine, but you don't need the matchTags, that would just be for if this specific brand might also be tagged e.g. shop/post_office (but not the others in this file, otherwise there are match groups for that). For example see:

"brands/shop/catalogue": [
{
"displayName": "Argos",
"id": "argos-3930ea",
"locationSet": {"include": ["gb", "ie"]},
"matchTags": [
"shop/department_store",
"shop/variety_store"
],

@@ -402,12 +402,13 @@
"displayName": "Post Office (UK)",
"id": "postoffice-a028f7",
"locationSet": {"include": ["gb"]},
"matchNames": ["Post Office"],
"matchTags": ["amenity/post_office"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

So like so:

Suggested change
"matchTags": ["amenity/post_office"],

Copy link
Collaborator

Choose a reason for hiding this comment

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

This has been done.

@bhousel
Copy link
Member

bhousel commented Oct 8, 2020

I'm unclear if matchNames will work against the name field if one isn't included in the entry. @bhousel ?

The matcher will already index all the variations of name, brand, operator tags so we don't need to include matchNames or matchTags here in this project.

I think someday I should update iD's validator to try these tags also, but as of today it just looks in name.

@peternewman
Copy link
Collaborator

The matcher will already index all the variations of name, brand, operator tags so we don't need to include matchNames or matchTags here in this project.

I think someday I should update iD's validator to try these tags also, but as of today it just looks in name.

Sorry I don't follow then, which matcher are you referring to? Or do you mean an internal NSI one to check for duplicates within NSI?

Do you mean if I have an entry:

        "amenity": "post_office",
        "brand": "Post Office",
        "name": "Foo Post Office"

That iD won't currently go "oh the brand matches, let's upgrade that and add some wiki tags"? I thought that was the whole point of making name not required within NSI. Presumably Post Office will still appear as a present in iD, it just won't ever offer to upgrade the tags on an existing node if it doesn't have a matching name?

@bhousel
Copy link
Member

bhousel commented Oct 9, 2020

Sorry I don't follow then, which matcher are you referring to? Or do you mean an internal NSI one to check for duplicates within NSI?

NSI started out with an internal matcher - the build scripts use this matcher to decide whether the things we collect from the OSM planet file are new to NSI or not (check out this comment from yesterday).

The NSI matcher gained sophistication so we now export it for use in other projects. iD's tag validator started using it last year, and that is what supplies many of the "upgrade tag" suggestions.

The way it works is: pass the matcher a key, value, "name", location, and it returns the items from NSI that match those things.
So,
match("amenity", "post_office", "Post Office", some [lng,lat] in UK)
will match the UK Post office (postoffice-a028f7 / Q1783168)
In iD, any missing or mismatched tags will be suggested as upgrades.

match("amenity", "post_office", "Foo Post Office", some [lng,lat] in UK)
will not match anything, so iD won't suggest any upgrades.

So there are a few possible enhancements that recent work in NSI has unblocked:

  • We can change iD so that it tries matching other identifying tags if they are present, like brand, network, operator.
  • name tag isn't required anymore, so we can remove it from NSI items where we don't want iD to try to upgrade it (e.g. UK post offices)

btw - I absolutely do need to do a better job documenting all this stuff, sorry! A lot of it is just known to me because it's code that I wrote, and that's not great. It is always ok to ask for more details on what the code is doing and why it works that way, so definitely let me know if I can make it more clear...

@bhousel bhousel merged commit de5c93a into osmlab:main Oct 9, 2020
@Cj-Malone Cj-Malone deleted the uk-po branch April 29, 2022 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
considering Not Actionable - still considering if this is something we want
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants