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

Publish a new release or otherwise ensure iD can import recent updates to the NSI #4543

Closed
rjw62 opened this issue Oct 20, 2020 · 39 comments
Closed
Labels
news Not Actionable - project news

Comments

@rjw62
Copy link
Contributor

rjw62 commented Oct 20, 2020

I see that this project hasn't published a release since v4.0.2 in April 2020. I assume that this is why iD hasn't updated its version of the NSI since that time. iD is thus not taking advantage of more recent updates to the NSI, and more importantly is continuing to entice mappers to make erroneous changes based on mistakes in that release of the NSI that have since been fixed.

I understand that there have been some changes to the output format of the NSI, which may cause problems for new releases and imports by iD. Nevertheless, I think something needs to be done between this project and iD to ensure that new releases can be made and will feed in to iD in a timely manner.

@bhousel
Copy link
Member

bhousel commented Oct 20, 2020

I agree, we should release a new version soon. The next published version of NSI will be a major release with breaking changes, so nothing will happen really quickly.. But I do expect to be able to push an update to iD in the next month or so.

@TheAdventurer64
Copy link
Contributor

Regarding this, GoMap has the newest changes to the NSI, while iD is still using outdated presets.

@quincylvania
Copy link
Contributor

@TheAdventurer64 That's nice for GoMap, but keep in mind that iD's update cycle isn't tied to any other editor. In open source, things get done when they get done 🤷

@bhousel
Copy link
Member

bhousel commented Mar 4, 2021

We're pretty close on this!
I deployed an iD with the preview NSI v5 code to: http://www.ideditor.org/nsi-v5/
Try it out and let me know if you find any odd validation or preset suggestions!

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 4, 2021

let me know if you find any odd validation or preset suggestions!

Hotel name validation seems to strict:
Bildschirmfoto 2021-03-04 um 18 53 32

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 4, 2021

Validator warning generally refers to an outdated or incomplete brand (Marke in German) instead of an outdated or incomplete network or operator:

image

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 4, 2021

Validator still wants to update supermarket name although brand tag is already correctly set:

Bildschirmfoto 2021-03-04 um 19 08 19

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 4, 2021

Nextbike operates several differently branded bike sharing systems across Europe. The validator seems to check for matching operator tags and wants to update to the wrong brand. For this example id should update to Edeka nextbike:

image

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 4, 2021

weird update suggestion for a stamp vending machine apparently based on matching operator, see here

Bildschirmfoto 2021-03-04 um 19 56 11

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 4, 2021

Wrong update suggestion for the network values of this relation. Maybe only match features with matching network tag and ignore matching operator tags.

Bildschirmfoto 2021-03-04 um 20 04 38

@bhousel
Copy link
Member

bhousel commented Mar 4, 2021

Thanks for testing @kjonosm !

Hotel name validation seems to strict:

Good catch, I was missing the preserveNames property on these. I'll push an update to the NSI dist/* files shortly.

Validator warning generally refers to an outdated or incomplete brand (Marke in German) instead of an outdated or incomplete network or operator:

Oh yeah, this is a localization issue that will be updated on the iD side eventually.. The new English source strings are a bit broader:

noncanonical_brand:
  message: "{feature} looks like a common feature with nonstandard tags"
  message_incomplete: "{feature} looks like a common feature with incomplete tags"
  reference: "Some features, for example retail chains or post offices, are expected to have certain tags in common."

Validator still wants to update supermarket name although brand tag is already correctly set:

Yea this is a lot like the Tesco example here. Do you think people would be ok if the validator offered tagging like name=Edeka+branch=Barnimstraße? The osm wiki does encourage it, so it seems like it would be a reasonable suggestion.

(will respond to others soon.. )

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 4, 2021

Do you think people would be ok if the validator offered tagging like name=Edeka+branch=Barnimstraße?

I can't say. I personally don't have a strong opinion about it but if branch=* was an "approved" key there might be more people willing to accept this tagging style.

@bhousel
Copy link
Member

bhousel commented Mar 4, 2021

I personally don't have a strong opinion about it but if branch=* was an "approved" key there might be more people willing to accept this tagging style.

From https://wiki.openstreetmap.org/wiki/Key:branch it looks like it's used >150,000 times, so it seems ok to me..

@bhousel
Copy link
Member

bhousel commented Mar 5, 2021

Thanks again @kjonosm ,
Capturing the feedback from above.. I've made a bunch of fixes and pushed a new version to http://www.ideditor.org/nsi-v5/

  • "Hotel validation too strict" - Fix missing Hotel/Motel preserveTags (fixed in NSI)
  • "Validator wants to upgrade Edeka supermarket name" - Now offers to split into name and branch (fixed in iD pull request)
  • "Weird update suggestion for a stamp vending machine" - This was improperly considering amenity/yes as an alternate match (Fixed in iD pull request, also added an entry for this vending machine in NSI)
  • "Wrong update suggestion for network" - Don't try any matching or upgrade if any of the namelike values contain semicolons (Fixed in iD pull request)
  • I still have to look into the nextbike issue

@bhousel
Copy link
Member

bhousel commented Mar 5, 2021

Nextbike operates several differently branded bike sharing systems across Europe. The validator seems to check for matching operator tags and wants to update to the wrong brand. For this example id should update to Edeka nextbike:

I looked into that nextbike a bit, you are correct that it's matching on the operator=nextbike tag, and it looks like the matcher understands these to be viable candidates (based on the operator/brand tags in NSI):
Screen Shot 2021-03-05 at 5 08 16 PM

"Edeka Nextbike" doesn't have a brand:wikidata tag, so it skips over that one and perfers the "metropolradruhr" one.

I notice that the bike share page has "nextbike" as a named exclude:

"properties": {
"path": "brands/amenity/bicycle_rental",
"exclude": {
"generic": [
"^bicycle rental$",
"^bike sharing$"
],
"named": ["^nextbike( gmbh)?$"]
}
},

So I might need to adjust something in NSI. I wasn't really expecting that things specifically excluded could also be used as tag values.

bhousel added a commit that referenced this issue Mar 5, 2021
@bhousel
Copy link
Member

bhousel commented Mar 5, 2021

So I might need to adjust something in NSI. I wasn't really expecting that things specifically excluded could also be used as tag values.

I made a change in the matcher to not index the excluded values (like "nextbike"), so that should take care of this issue!

@kjonosm thanks again, and let me know if things look better on http://www.ideditor.org/nsi-v5/

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 6, 2021

I don't know what happens here but removing the surveillance tag seems too strict.

Edit: I come across the surveillance tag quite often (in Germany). So suggesting to just remove it really seems like a bad idea.

image

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 6, 2021

Not really a validation issue but is it possible/desirable to show the operator logo if a brand logo is missing? See here for example, where it would be nice to see a DHL logo when selecting the object in iD. This might also be an improvement for networks with missing logos...
image

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 6, 2021

Please mention the feature type in validator text to make it less confusing for users. Here it would be useful to give a hint that the issue is about a mailbox (Briefkasten in German).

image

image

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 6, 2021

The name suggestion for car-sharing stations seems too strict as most stations tend to have a unique name (like amazon lockers). See here for example. For this operator I already removed the name tag in nsi.

image

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 6, 2021

It seems like id uses the nsi displayName value for the feature label, e.g. "Postbank (Germany)" in this example. I stumbled upon the brackets and wondered whether for iD it made more sense to instead just use the feature's brand value to make it less confusing for users?

image

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 6, 2021

Many transit routes in nsi have an operator tag that often wrongly duplicates the network tag. I cleaned up lots of these in nsi already but there still are many routes where the validator suggests to remove valid operator data, like in this example.

This happens because (as recommended by osm wiki) in many countries network is used to tag the transit district often consisting of several different transport companies licensed to operate in.

image

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 6, 2021

This example shows a shop where the branch name doesn't denote the location but the franchisee. In this context branch=Klein looked odd to me and made me wonder, whether it might generally be better to use the entire name for branch as in branch=Expert Klein despite slightly deviating from the osm-wiki recommendation.

image

Edit: Two more instances I came across, where just cutting off the branch name doesn't look right:

@bhousel
Copy link
Member

bhousel commented Mar 8, 2021

Thanks @kjonosm, I'll work through these issues today:

  • Don't remove man_made=surveillance tag (same issue we have with pharmacy tag here Name-suggestion-index v6 openstreetmap/iD#8305 (comment))
  • DHL Logo fallback? We can't easily fallback to the operator logo, (thought about it more, we can when we generate the presets). I can at least make sure Wikidata has something for DHL Packstation
  • Please mention the feature type in validator text (I agree!)
  • NSI displayName value for the feature label, e.g. "Postbank (Germany)" - We don't have an easy way to change this, but we could have the displayName in the local language, like "Postbank (Deutchland)" so that it looks less weird to a German speaker?
  • The operator/network thing - (I don't really understand it) - thanks for explaining, I did this in operator tags are often wrong in the transit tree #4944
  • @kjonosm - Where splitting of name into name/branch looks wrong - are these examples where we should just leave name alone? (like Pubs, Hotels, Apple Stores, Amazon Lockers)? We can do that too. I don't know these brands well enough to decide it..

bhousel added a commit to openstreetmap/iD that referenced this issue Mar 8, 2021
bhousel added a commit that referenced this issue Mar 8, 2021
This doesn't get all of them, but hopefully it gets most obvious ones
(re: #4543 (comment))
@kjonosm
Copy link
Collaborator

kjonosm commented Mar 8, 2021

The operator/network thing - (I don't really understand it)

For each transit network collected from the planet, the operator value is automatically added by just duplicating the network value. In many cases this is a mistake and leads to wrong data. See e.g. following networks where the operator tag in nsi is just a copy of the network tag, deviating from the actual tagging in OSM you can compare in the given overpass turbo links:

iD validator now suggests to update the existing and correctly tagged operator value with this wrongly collected operator value.

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 8, 2021

are these examples where we should just leave name alone?

Hard to say. I don't really mind the shortened name tag. It's the branch tag that somehow looks odd to me, hence my suggestion to just copy the entire string to the branch value.
In addition, for the examples where the franchisee makes part of the name (Intersport Voswinkel, Expert Klein) it's technically not a (unique) branch name as both franchisees to my knowledge own more than one shop.

@bhousel
Copy link
Member

bhousel commented Mar 8, 2021

For each transit network collected from the planet, the operator value is automatically added by just duplicating the network value. In many cases this is a mistake and leads to wrong data. See e.g. following networks where the operator tag in nsi is just a copy of the network tag, deviating from the actual tagging in OSM you can compare in the given overpass turbo links:

Ah yeah I see now. Since so many are wrong, I think I'm just going to remove all the operator values from the transit tree, and people will need to put them back manually. This is annoying and I feel bad about it, but at least it will prevent the validator from pushing wrong tags. I'm going to open a new issue for it, so everyone knows..

@bhousel
Copy link
Member

bhousel commented Mar 8, 2021

I pushed the latest work to https://ideditor.org/nsi-v5 - let me know what you think!

The way I'm looking at the name/branch thing is - splitting the name up is better than overwriting the name because then data doesn't get lost. I'd like to let people test for another week or so and see what people think.

We can also definitely leave the name alone on a per-category or per-item basis..

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 9, 2021

Weird validator message for this shop (feature name comes up twice).

image

Edit: Same here

@bhousel
Copy link
Member

bhousel commented Mar 9, 2021

@kjonosm do you mean how the message says "Saturn Saturn..." (which I agree is weird).
I think this is just the preset and the name appearing together in a weird way. We can display the non-verbose version of the message if the preset is a suggestion preset.

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 9, 2021

We can display the non-verbose version of the message if the preset is a suggestion preset.

Yes, that should work. Thanks!

@bhousel
Copy link
Member

bhousel commented Mar 10, 2021

Yes, that should work. Thanks!

Great - it looks like this now.. I agree it's much better 👍

Screen Shot 2021-03-10 at 10 35 30 AM

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 10, 2021

The Nextbike issue shows up again, see this node
I have to say I don't really know how to properly tag these nextbike schemes myself.
They run several different business models, see their website

image

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 11, 2021

Weird validator message at bottom of the screenshot for this feature

Bildschirmfoto 2021-03-11 um 13 24 50

bhousel added a commit to openstreetmap/iD that referenced this issue Mar 11, 2021
@bhousel
Copy link
Member

bhousel commented Mar 11, 2021

Thanks again @kjonosm - I think I've fixed the 2 issues above, can you refresh https://ideditor.org/nsi-v5 and test again when you have some time?

  • I fixed the weird error message (incidentally, it is a warning that this feature shouldn't be a point on the edge of another feature - I'm not sure whether this message is really a fair issue, as I know a few mappers like to put POIs on building entrances - but in any case extracting the shop to its own standalone point fixes it)
  • after nextbike is recognized as a generic name, the matcher will stop looking (what it was doing before was falling back to a match on brand:wikidata qid.)

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 12, 2021

The validator doesn't suggest a branch tag for this bank. Maybe the comma in the name tag is causing this?

image

bhousel added a commit to openstreetmap/iD that referenced this issue Mar 12, 2021
This now breaks the name into fragments and reruns the fragments against the NSi matcher
rather than using flaky regular expressions. Has a few advantages:
(re: osmlab/name-suggestion-index#4543 (comment))
@bhousel
Copy link
Member

bhousel commented Mar 12, 2021

Maybe the comma in the name tag is causing this?

Yeah, that was it - I rewrote the name/branch splitting code this morning to make it more reliable.

@kjonosm
Copy link
Collaborator

kjonosm commented Mar 15, 2021

name/branch splitting needs some adjustment. Here the code creates an empty value...

image

@bhousel
Copy link
Member

bhousel commented Mar 24, 2021

I've been doing a bunch of testing with this the past few days, and I think it's about as good as it will get.. NSI v5 has been published, and the test branch below will pull the latest version.. I'd love for more people to test here and let me know if you notice any issues:
https://ideditor.org/nsi-v5

@bhousel bhousel closed this as completed Mar 24, 2021
@Identitaet Identitaet pinned this issue May 31, 2021
@Identitaet Identitaet unpinned this issue May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
news Not Actionable - project news
Projects
None yet
Development

No branches or pull requests

5 participants