Modify

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#17100 closed enhancement (fixed)

complain about descriptive names and offer to fix them

Reported by: mkoniecz Owned by: Klumbumbus
Priority: normal Milestone: 19.03
Component: Core validator Version:
Keywords: template_report Cc:

Description (last modified by Klumbumbus)

What steps will reproduce the problem?

  1. Create a closed way with amenity=parking and name=parking
  2. Run validator

What is the expected result?

Validator complains about undesirable descriptive name and proposes to remove it.

What happens instead?

Nothing

Please provide any additional information below. Attach a screenshot if possible.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2018-12-11 00:24:50 +0100 (Tue, 11 Dec 2018)
Build-Date:2018-12-11 02:32:21
Revision:14551
Relative:URL: ^/trunk

Identification: JOSM/1.5 (14551 en) Linux Ubuntu 16.04.5 LTS
Memory Usage: 497 MB / 869 MB (294 MB allocated, but free)
Java version: 1.8.0_191-b12, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: :0.0 1920x1080
Maximum Screen Size: 1920x1080
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (34535)
+ buildings_tools (34724)
+ continuosDownload (82)
+ imagery_offset_db (34641)
+ measurement (34529)
+ reverter (34552)
+ todo (30306)

Last errors/warnings:
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet

Attachments (1)

invalid parking names.png (51.9 KB ) - added by mkoniecz 5 years ago.

Download all attachments as: .zip

Change History (50)

by mkoniecz, 5 years ago

Attachment: invalid parking names.png added

comment:1 by mkoniecz, 5 years ago

Attachment has history how many such names were present according to the http://taghistory.raifer.tech/ tool

comment:2 by GerdP, 5 years ago

This is quite special. Similar problem that I came across: building=house with name=house or shop=* with name=Shop.
Do you think about a test that would flag any name=v where this v also appears for another tag (excluding all other name tags
like name:de or alt_name etc), maybe ignoring the case so that Shop and shop do both match?

comment:3 by mkoniecz, 5 years ago

This is quite special. Similar problem that I came across: building=house with name=house or shop=* with name=Shop

Yes, my plan was to propose this obvious and relatively popular case and check whatever this rule type would be considered as OK. And in case of inclusion later propose some other potential matches from my list that are added so often that I am unable to process them on my own.

Do you think about a test that would flag any name=v where this v also appears for another tag (excluding all other name tags
like name:de or alt_name etc)

I worry that it may have many false positives. For start, in my own validator script I already exclude:

  • places (there are places with all kinds of bizarre name, like city in Turkey called Batman - and probably somewhere there is a village actually called Parking)
  • restaurants (restaurants sometimes have deliberately crazy and bizarre names, restaurant named Parking would not be outside what is normal)
  • peaks (some peaks also have truly weird names, I expect the same for other natural features but they are not mapped so often so I still have to run into one)

Sometimes descriptive name indicates that editor got confused (like tourism=attraction, name=hotel) and in such cases simply removing name is not helping.

There are also objects tagged solely with name=parking - for such cases removing names is not helping at all.

Overall I think that it would be safer to start from known popular descriptive names and add more manually rather than start from generic rule and add exclusions. I think that first would achieve the same results with less work and less false positives exposed to mappers. But the second approach is also OK.

Last edited 5 years ago by mkoniecz (previous) (diff)

comment:4 by mkoniecz, 5 years ago

Description: modified (diff)

comment:5 by majkaz, 5 years ago

I would love to have this: ...Do you think about a test that would flag any name=v ...

implemented like this: use the preset values, and for object (node/way/relation) with tag, for example building, the validator complains about any "preset" value of "building" used for name of the same object.
Just for tourism=attraction I would include all the possible values from presets. Reasoning: first in mobile apps (maps.me / OsmAnd) and gets often misused as catchall. Others might know other suspected tags, where this is the same / similar case.

What I have seen, all the remaining errors fall under the "object has only name and no other tags" test.

If testing for all possible values of all keys, I would set this as secondary test, getting "suspicious names". But this should be possibly an extra an user can disable without disabling the first one.

comment:6 by GerdP, 5 years ago

Owner: changed from team to GerdP

I'll think about this. Probably no implementation this month.

comment:7 by mkoniecz, 5 years ago

I can provide patch with set of simple rules (amenity=parking + name=parking, leisure=pitch + name=pitch, leisure=pitch + sport=soccer + name=boisko etc).

Is there chance that it would be merged?

comment:8 by GerdP, 5 years ago

Hard to say without knowing the patch ;)
The first example occurs quite often, but your other two examples only a few times. I don't think that we should add a test for each dubious combination.

comment:9 by mkoniecz, 5 years ago

OK, that no longer would be a good example.

Is 800+ cases like for name=playground/name=Playground enough ( http://overpass-turbo.eu/s/GyP )?

Or 900+ for name=shop/name=Shop ( http://overpass-turbo.eu/s/GyN )?

Though 13k for name=building ( http://overpass-turbo.eu/s/GyR ) certainly qualifies.

Last edited 5 years ago by mkoniecz (previous) (diff)

comment:10 by GerdP, 5 years ago

Yes, I think so, too.

comment:11 by mkoniecz, 5 years ago

There is also name=kiosk shop=kiosk with 891 uses http://overpass-turbo.eu/s/Gz1

comment:12 by mkoniecz, 5 years ago

Questions:

  • Should it go in group "deprecated tagging" or create a new one like "descriptive names"?
  • Is there a better way to ignore case in matches?

Code below is not a patch and is not tested, I just looked at validator rules - though feedback is welcomed

*[name=parking][amenity=parking],
*[name=Parking][amenity=parking]
 {
  throwWarning: tr("{0} descriptive names are unneded", "{0.tag}");
  group: tr("deprecated tagging");
  suggestAlternative: "it is enough to use amenity=parking, name=parking is unnecessary";
  fixRemove: "name";
  assertMatch: "way name=parking amenity=parking";
  assertMatch: "way name=parking amenity=Parking";
  assertMatch: "node name=parking amenity=parking";
  assertMatch: "node name=parking amenity=Parking";
  assertMatch: "relation name=parking amenity=parking";
  assertMatch: "relation name=parking amenity=Parking";
  assertNoMatch: "way name=parking";
}

Last edited 5 years ago by Don-vip (previous) (diff)

comment:13 by mkoniecz, 5 years ago

And cemeteries looks like a good candidates for a similar check: http://overpass-turbo.eu/s/Gzr http://overpass-turbo.eu/s/Gzs

in reply to:  12 comment:14 by Klumbumbus, 5 years ago

Replying to mkoniecz:

  • Should it go in group "deprecated tagging" or create a new one like "descriptive names"?

I suggest:

*[name=building][building],
...
...
...
*[name=parking][amenity=parking] {
  throwWarning: tr("{0}", "{0.tag}");
  group: tr("descriptive name");
  fixRemove: "name";
  assertMatch: ...
}

(and it should be added in unnecessary.mapcss)


  • Is there a better way to ignore case in matches?

This seems to be a bug. wiki:Help/Styles/MapCSSImplementation states several times that without quotes it is case insensitive.

comment:15 by mkoniecz, 5 years ago

@Klumbumbus

Thanks for idea of putting into one rule and directing me to unnecessary.mapcss and to docs that it is case insensitive (I have not tested it, just stupidly assumed that it is case sensitive).

comment:16 by Klumbumbus, 5 years ago

I tested it and it doesn't work but I think it used to work in the past.
(edit: ... the case insensitiveness)

Last edited 5 years ago by Klumbumbus (previous) (diff)

comment:17 by mkoniecz, 5 years ago

My current plan for failed insensitivity is to:

*[name=parking][amenity=parking],
*[name=playground][leisure=playground],
*[name=shop][shop][shop!=no],
*[name=building][building][building!=no],
*[name=kiosk][shop=kiosk],
*[name=cemetery][amenity=graveyard],
*[name=cemetery][landuse=cemetery],
*[name=cmentarz][amenity=graveyard],
*[name=cmentarz][landuse=cemetery]
 {
  throwWarning: tr("{0}", "{0.tag}");
  group: tr("descriptive name");
  fixRemove: "name";
  assertMatch: "way name=parking amenity=parking";
  assertMatch: "way name=parking amenity=Parking";
  assertMatch: "node name=parking amenity=parking";
  assertMatch: "node name=parking amenity=Parking";
  assertMatch: "relation name=parking amenity=parking";
  assertMatch: "relation name=parking amenity=Parking";
  assertNoMatch: "way name=parking";
  assertMatch: "relation name=PLAYGROUND leisure=playground";
  assertMatch: "node name=PLaYGrOUNd leisure=playground";
  assertNoMatch: "way name=playground";
  assertMatch: "node name=shop shop=whatever";
  assertNoMatch: "node name=shop shop=no";
  assertNoMatch: "way name=shop leisure=playground";
  assertMatch: "way name=building building=yes";
  assertNoMatch: "way building=yes";
  assertMatch: "way name=kiosk building=yes shop=kiosk";
  assertNoMatch: "way name=kiosk building=yes";
  assertMatch: "way name=cemetery amenity=graveyard";
  assertMatch: "way name=cmentarz amenity=graveyard";
  assertMatch: "way name=Cmentarz amenity=graveyard";
  assertNoMatch: "way name=kiosk amenity=graveyard";
}
Last edited 5 years ago by Don-vip (previous) (diff)

comment:18 by mkoniecz, 5 years ago

Current proposed validator code below. Now I need to find git mirror (found at https://github.com/openstreetmap/josm ).

*[name=Parking][amenity=parking],
*[name=Playground][leisure=playground],
*[name=Shop][shop][shop!=no],
*[name=Building][building][building!=no],
*[name=Kiosk][shop=kiosk],
*[name=Cemetery][amenity=graveyard],
*[name=Cemetery][landuse=cemetery],
*[name=Cmentarz][amenity=graveyard],
*[name=Cmentarz][landuse=cemetery],
# rules above are temporary workaround for https://josm.openstreetmap.de/ticket/17398#ticket
*[name=parking][amenity=parking],
*[name=playground][leisure=playground],
*[name=shop][shop][shop!=no],
*[name=building][building][building!=no],
*[name=kiosk][shop=kiosk],
*[name=cemetery][amenity=graveyard],
*[name=cemetery][landuse=cemetery],
*[name=cmentarz][amenity=graveyard],
*[name=cmentarz][landuse=cemetery]
 {
  throwWarning: tr("{0}", "{0.tag}");
  group: tr("descriptive name");
  fixRemove: "name";
  assertMatch: "way name=parking amenity=parking";
  assertMatch: "way name=Parking amenity=parking";
  assertMatch: "node name=parking amenity=parking";
  assertMatch: "node name=Parking amenity=parking";
  assertMatch: "relation name=parking amenity=parking";
  assertMatch: "relation name=parking amenity=Parking";
  assertNoMatch: "way name=parking";
  assertMatch: "relation name=PLAYGROUND leisure=playground";
  assertMatch: "node name=PLaYGrOUNd leisure=playground";
  assertNoMatch: "way name=playground";
  assertMatch: "node name=shop shop=whatever";
  assertNoMatch: "node name=shop shop=no";
  assertNoMatch: "way name=shop leisure=playground";
  assertMatch: "way name=building building=yes";
  assertNoMatch: "way building=yes";
  assertMatch: "way name=kiosk building=yes shop=kiosk";
  assertNoMatch: "way name=kiosk building=yes";
  assertMatch: "way name=cemetery amenity=graveyard";
  assertMatch: "way name=cmentarz amenity=graveyard";
  assertMatch: "way name=Cmentarz amenity=graveyard";
  assertNoMatch: "way name=kiosk amenity=graveyard";
}
Last edited 5 years ago by Don-vip (previous) (diff)

comment:20 by mkoniecz, 5 years ago

Summary: complain about name=parking or name=Parking for amenity=parkingcomplain about name=parking or name=Parking for amenity=parking [PATCH]

comment:21 by Klumbumbus, 5 years ago

Milestone: 19.03

comment:22 by GerdP, 5 years ago

@Klumbusbus: Please take over. I thought about an implementation in Java, but that would probably produce too many false positives.

comment:23 by Klumbumbus, 5 years ago

I was waiting if #17398 will be fixed first :) but I just noticed Don-vip commented there.

comment:24 by mkoniecz, 5 years ago

I closed it by editing wiki and wontfixing it per Don-vip comment.

comment:25 by mkoniecz, 5 years ago

I tried moving to regexp but

*[name=~/^parking$/][amenity=parking],
*[name=~/^playground$/][leisure=playground],
*[name=~/^shop$/][shop][shop!=no],
*[name=~/^building$/][building][building!=no],
*[name=~/^kiosk$/][shop=kiosk],
*[name=~/^cemetery$/][amenity=graveyard],
*[name=~/^cemetery$/][landuse=cemetery],
*[name=~/^cmentarz$/][amenity=graveyard],
*[name=~/^cmentarz$/][landuse=cemetery]
 {
  throwWarning: tr("{0}", "{0.tag}");
  group: tr("descriptive name");
  fixRemove: "name";
  assertMatch: "way name=parking amenity=parking";
  assertMatch: "way name=Parking amenity=parking";
  assertMatch: "node name=parking amenity=parking";
  assertMatch: "node name=Parking amenity=parking";
  assertNoMatch: "node name=Parking_with_suffix amenity=parking";
  assertNoMatch: "node name=Megaparking amenity=parking";
  assertMatch: "relation name=parking amenity=parking";
  assertMatch: "relation name=parking amenity=Parking";
  assertNoMatch: "way name=parking";
  assertMatch: "relation name=PLAYGROUND leisure=playground";
  assertMatch: "node name=PLaYGrOUNd leisure=playground";
  assertNoMatch: "way name=playground";
  assertMatch: "node name=shop shop=whatever";
  assertNoMatch: "node name=shop shop=no";
  assertNoMatch: "way name=shop leisure=playground";
  assertMatch: "way name=building building=yes";
  assertNoMatch: "way building=yes";
  assertMatch: "way name=kiosk building=yes shop=kiosk";
  assertNoMatch: "way name=kiosk building=yes";
  assertMatch: "way name=cemetery amenity=graveyard";
  assertMatch: "way name=cmentarz amenity=graveyard";
  assertMatch: "way name=Cmentarz amenity=graveyard";
  assertNoMatch: "way name=kiosk amenity=graveyard";
} 

Causes throwWarning: tr("{0}", "{0.tag}"); to stop working.

comment:26 by Klumbumbus, 5 years ago

It works with brackets: *[name=~/^(parking)$/][amenity=parking], but then again not anymore with embedded (?i)

comment:27 by Klumbumbus, 5 years ago

Not sure if it is the best solution but *[name][name=~/^(?i)(parking)$/][amenity=parking], does the trick.

Last edited 5 years ago by Klumbumbus (previous) (diff)

comment:28 by mkoniecz, 5 years ago

Ready, unit tests now all pass! Thanks for the help!

*[name][name=~/^(?i)(parking)$/][amenity=parking],
*[name][name=~/^(?i)(playground)$/][leisure=playground],
*[name][name=~/^(?i)(shop)$/][shop][shop!=no],
*[name][name=~/^(?i)(building)$/][building][building!=no],
*[name][name=~/^(?i)(kiosk)$/][shop=kiosk],
*[name][name=~/^(?i)(cemetery)$/][amenity=graveyard],
*[name][name=~/^(?i)(cemetery)$/][amenity=cemetery],
*[name][name=~/^(?i)(cmentarz)$/][amenity=graveyard],
*[name][name=~/^(?i)(cmentarz)$/][amenity=cemetery]
 {
  throwWarning: tr("{0}", "{0.tag}");
  group: tr("descriptive name");
  fixRemove: "name";
  assertMatch: "way name=parking amenity=parking";
  assertMatch: "way name=Parking amenity=parking";
  assertMatch: "node name=parking amenity=parking";
  assertMatch: "node name=Parking amenity=parking";
  assertNoMatch: "node name=Parking_with_suffix amenity=parking";
  assertNoMatch: "node name=Megaparking amenity=parking";
  assertMatch: "relation name=parking amenity=parking type=multipolygon";
  assertMatch: "relation name=Parking amenity=parking type=multipolygon";
  assertNoMatch: "way name=parking";
  assertMatch: "relation name=PLAYGROUND leisure=playground type=multipolygon";
  assertMatch: "node name=PLaYGrOUNd leisure=playground";
  assertNoMatch: "way name=playground";
  assertMatch: "node name=shop shop=whatever";
  assertNoMatch: "node name=shop shop=no";
  assertNoMatch: "way name=shop leisure=playground";
  assertMatch: "way name=building building=yes";
  assertNoMatch: "way building=yes";
  assertMatch: "way name=kiosk building=yes shop=kiosk";
  assertNoMatch: "way name=kiosk building=yes";
  assertMatch: "way name=cemetery amenity=graveyard";
  assertMatch: "way name=cmentarz amenity=graveyard";
  assertMatch: "way name=Cmentarz amenity=graveyard";
  assertNoMatch: "way name=kiosk amenity=graveyard";
} 

comment:29 by Klumbumbus, 5 years ago

Resolution: fixed
Status: newclosed

In 14884/josm:

fix #17100 - warn about descriptive names (patch by mkoniecz)

comment:30 by Klumbumbus, 5 years ago

(I added Spielplatz and Parkplatz)

comment:31 by GerdP, 5 years ago

Maybe add a test for building=house + name=house, appears ~ 1100 times

comment:32 by Klumbumbus, 5 years ago

name=house and name=House are 7455 together.

comment:33 by Klumbumbus, 5 years ago

In 14885/josm:

see #17100 - warn about name=house too

comment:34 by GerdP, 5 years ago

highway=* and name=jalan or Jalan is also a good candidate, jalan means road/street

comment:35 by Klumbumbus, 5 years ago

Resolution: fixed
Status: closedreopened

comment:36 by GerdP, 5 years ago

There are also > 20000 nodes with
emergency=fire_hydrant
fire_hydrant:type=underground
name=地下式消防栓

in reply to:  36 comment:37 by Klumbumbus, 5 years ago

Replying to GerdP:

There are also > 20000 nodes with
emergency=fire_hydrant
fire_hydrant:type=underground
name=地下式消防栓

Maybe an import?

comment:38 by GerdP, 5 years ago

Yes, possible. It is used in a rather small area. I'll have a look at the changeset and maybe add a comment.

Another candidate:
building=* and name=Rumah
Rumah means house

comment:39 by mkoniecz, 5 years ago

fix #17100 - warn about descriptive names (patch by mkoniecz)

Note that not only warns - it also offers to autoremove names.

That was my reason for not including name=house as there may be some value in moving this info to building tag.

Though data added by somebody using name tag is suspect anyway, so throwing it away is probably also OK.

comment:40 by mkoniecz, 5 years ago

Yes, possible. It is used in a rather small area. I'll have a look at the changeset and maybe add a comment.

Looks like a yet another import. I commented in https://www.openstreetmap.org/changeset/68120342#map=11/25.0792/121.6027&layers=N (made 19 hours ago).

comment:41 by mkoniecz, 5 years ago

(I added Spielplatz and Parkplatz)

Thanks for merging and adding other popular descriptive names!

Version 0, edited 5 years ago by mkoniecz (next)

comment:42 by Klumbumbus, 5 years ago

Ticket #17476 has been marked as a duplicate of this ticket.

comment:43 by Klumbumbus, 5 years ago

from #17476:
man_made=silo + name=Silo
http://overpass-turbo.eu/s/H5q - 245 cases in many places across the world

comment:44 by mkoniecz, 5 years ago

untested code for entries that IMHO certainly should be included:

*[name][name=~/^(?i)(silo)$/][man_made=silo],
*[name][name=~/^(?i)(silo)$/][building=silo],
*[name][name=~/^(?i)(地下式消防栓)$/][emergency=fire_hydrant],
*[name][name=~/^(?i)(rumah)$/][building=house],

and tests

  assertMatch: "way name=silo man_made=silo";
  assertMatch: "way name=Silo man_made=silo building=silo";
  assertMatch: "way name=Silo building=silo";
  assertNoMatch: "way name=kiosk building=silo";
  assertMatch: "way name=地下式消防栓 emergency=fire_hydrant";
  assertMatch: "way name=Rumah building=house";
  assertMatch: "way name=RuMaH building=house";

I am unsure about

name=House/Rumah on building=yes

BTW, it may be worth moving https://josm.openstreetmap.de/browser/josm/trunk/data/validator/deprecated.mapcss#L456 here (*[name="АЗС"][amenity=fuel],)

highway=* and name=jalan or Jalan is also a good candidate, jalan means road/street

4632 objects :( - see http://overpass-turbo.eu/s/H7x

I am unsure whatever highway=* is not too gready, maybe match on known highway values?

comment:45 by mkoniecz, 5 years ago

Summary: complain about name=parking or name=Parking for amenity=parking [PATCH]complain about descriptive names and offer to fix them

comment:46 by Klumbumbus, 5 years ago

Owner: changed from GerdP to Klumbumbus
Status: reopenednew

(hm, changing the owner of the ticket without changing the status seems impossible)

comment:47 by Klumbumbus, 5 years ago

Resolution: fixed
Status: newclosed

In 14911/josm:

fix #17100, see #17471 - add name=silo|rumah|jalan to descriptive name warning, move АЗС there too, disable autofix for cases where the value of the name tag could be the value of the other tag (manual fix by human required in these cases)

comment:48 by Klumbumbus, 5 years ago

I didn't add name=地下式消防栓 as this is only a local problem. All other comments after comment:33 should be considered with r14911.

comment:49 by Klumbumbus, 4 years ago

Description: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Klumbumbus.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.