Modify

Opened 6 years ago

Closed 6 years ago

#8687 closed enhancement (fixed)

Warn about letters in number only values

Reported by: skyper Owned by: team
Priority: normal Milestone: 14.01
Component: Core validator Version:
Keywords: number value warning Cc: RicoZ

Description

Looking at the values for height on taginfo, I find a lot of values containing not only numbers but unneeded letters.

There are some tags which only accept numbers as value or not more than some letter combinations:

  • layer
  • level
  • height
  • (max)height*
  • minclearance
  • width
  • (max)width
  • max-/mintrackwidth
  • maxspeed*
  • voltage
  • frequency
  • gauge

and probably some more.

A warning if the value does not only contain a valid number would be nice. Probably needs to categories:

  1. a warning about letters
  2. a warning about wrong decimal separator (e.g. , or :)

r5935

Attachments (8)

patch_8687_1.diff (3.6 KB) - added by dommage 6 years ago.
Regexp patterns for some measurement tags.
patch_8687_2.diff (3.9 KB) - added by dommage 6 years ago.
Updated patch including level and negative layers.
patch_8687_3.diff (3.9 KB) - added by dommage 6 years ago.
Corrected level comment and allow decimal component for incline.
patch_8687_4.diff (4.0 KB) - added by dommage 6 years ago.
Updated with results of testing.
regexp-data.json (3.2 KB) - added by dommage 6 years ago.
JSON test data for regular expressions.
regtest.go (1.0 KB) - added by dommage 6 years ago.
GO tool for running regexp tests.
patch_8687_5.diff (12.1 KB) - added by dommage 6 years ago.
JUnit test first version.
8687_test.patch (5.5 KB) - added by simon04 6 years ago.

Download all attachments as: .zip

Change History (53)

Changed 6 years ago by dommage

Attachment: patch_8687_1.diff added

Regexp patterns for some measurement tags.

comment:1 Changed 6 years ago by dommage

The patch diff contains a set of regular expressions for validating measurement values.
Some are fairly exact (voltage, layer) where the wiki and taginfo support it. The rest attempt to be more forgiving.
I did not find any references to minclearance or mintrackwidth (or variations) in taginfo.

It might be worth looking into creating a new test type to look at 'value' keys. There are many cases where the wiki gives consistent rules:

  • no thousands separator
  • use . for decimal place
  • semi-colon separate multiple values
  • space before units

comment:2 Changed 6 years ago by Don-vip

Summary: Warn about letters in number only values[PATCH] Warn about letters in number only values

comment:3 Changed 6 years ago by skyper

See also #8964.
For incline=* there should be no space between the value and the addon (% is not really a unit).

comment:4 Changed 6 years ago by skyper

level can also have negative values. Think about cellars and the underground.

Changed 6 years ago by dommage

Attachment: patch_8687_2.diff added

Updated patch including level and negative layers.

comment:5 Changed 6 years ago by dommage

The patch_8687_2.diff description should read "Updated patch including incline and negative levels."

comment:6 Changed 6 years ago by skyper

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

comment:7 in reply to:  5 ; Changed 6 years ago by skyper

Replying to dommage:

The patch_8687_2.diff description should read "Updated patch including incline and negative levels."

Thanks

Two smaller issues.

  • Please fix the comment for level
  • as percentage is a multiplier a value between 0 and 1 with several digits (zero to three) and without postfix should be allowed as stated on #8964.

comment:8 in reply to:  7 ; Changed 6 years ago by dommage

Replying to skyper:

Replying to dommage:

  • as percentage is a multiplier a value between 0 and 1 with several digits (zero to three) and without postfix should be allowed as stated on #8964.

#8964 says "if only numbers are added the range would be between zero and one (maybe two)" which to me sounds like radians (which would imply 'rad' for units). I didn't see any mention of that in the wiki/talk page and would prefer not to add a third way of expressing inclines. Mainly because I checked taginfo for incline (the first 5 pages) and while I saw percentages with a decimal component (12.3% for example) I did not see any naked (no %-sign) numbers less than 1. It doesn't look as though anyone is currently doing this.

Changed 6 years ago by dommage

Attachment: patch_8687_3.diff added

Corrected level comment and allow decimal component for incline.

comment:9 Changed 6 years ago by AlfonZ

I'm wondering if it should check also values on nodes.

comment:10 in reply to:  9 Changed 6 years ago by dommage

Replying to AlfonZ:

I'm wondering if it should check also values on nodes.

From the wiki: "The use of this tag has limited (if any) usage, as nodes do not have a direction."
According to taginfo about 1% of uses of the tag are on nodes, but I'm not sure what they're on. Signs would make some sense, but the information is definitely more useful to applications (and probably maps) on the way.

comment:11 in reply to:  8 Changed 6 years ago by skyper

Replying to dommage:

#8964 says "if only numbers are added the range would be between zero and one (maybe two)" which to me sounds like radians (which would imply 'rad' for units). I didn't see any mention of that in the wiki/talk page and would prefer not to add a third way of expressing inclines. Mainly because I checked taginfo for incline (the first 5 pages) and while I saw percentages with a decimal component (12.3% for example) I did not see any naked (no %-sign) numbers less than 1. It doesn't look as though anyone is currently doing this.

Ok, I can live with it and if no one is using it we shouldn't bother. Just to get it clear: I was talking about the meaning of percent which is equal to "multiply by .01", e.g. 8% is equal to .08 .

comment:12 in reply to:  9 Changed 6 years ago by skyper

Replying to AlfonZ:

I'm wondering if it should check also values on nodes.

Why not ?

  • Except of incline which should not be used at all on nodes. (Sounds like a different warning.)

comment:13 Changed 6 years ago by dommage

gauge, width, maxspeed, layer, voltage frequency and incline are all overwhelmingly applied to ways (on the order of 99%); enough that it is probably worth sending the user to the wiki to check if they're using the key correctly. I have modified the others in patch_8687_4.diff.

Changed 6 years ago by dommage

Attachment: patch_8687_4.diff added

Updated with results of testing.

Changed 6 years ago by dommage

Attachment: regexp-data.json added

JSON test data for regular expressions.

Changed 6 years ago by dommage

Attachment: regtest.go added

GO tool for running regexp tests.

comment:14 Changed 6 years ago by Don-vip

Thanks for the test, but is it possible for you to create a Junit test instead ? We are currently improving them a bit, we do not want another new langague for tests :)

Test may be created in test/unit folder while the JSON file can be put in test/data.

comment:15 Changed 6 years ago by Don-vip

Owner: changed from team to skyper
Status: newneedinfo

comment:16 Changed 6 years ago by Don-vip

Owner: changed from skyper to dommage

Changed 6 years ago by dommage

Attachment: patch_8687_5.diff added

JUnit test first version.

comment:17 Changed 6 years ago by dommage

JUnit test for TagChecker regular expressions added.
It uses the Google gson-2.2.4 library for JSON parsing. If there was an existing JSON library I missed it. I can attach the library if that is acceptable/desireable (it is 190k).

Critiques of the changes/additions are welcome. Java is not my primary language.

(Please excuse the double-post of the diff.)

comment:18 Changed 6 years ago by Don-vip

Owner: changed from dommage to team
Status: needinfonew

comment:19 Changed 6 years ago by skyper

Would be a nice improvement.

comment:20 Changed 6 years ago by stoecker

In 6251/josm:

see #8687 - add validator tests for number values

comment:21 Changed 6 years ago by stoecker

Skipped the tests for now - Do we really need a new parser for this? Wouldn't it be easier to simply convert the JSON data file into Java code and directly include it?

comment:22 Changed 6 years ago by stoecker

Summary: [PATCH] Warn about letters in number only values[PATCH - unit test needs rework] Warn about letters in number only values

comment:23 Changed 6 years ago by Don-vip

Agreed, there is no need for the JSON data.

comment:24 Changed 6 years ago by anonymous

Would it be possible to exclude maxheight=none from this check? See http://taginfo.openstreetmap.org/keys/maxheight#values

comment:25 in reply to:  24 Changed 6 years ago by skyper

Replying to anonymous:

Would it be possible to exclude maxheight=none from this check? See http://taginfo.openstreetmap.org/keys/maxheight#values

Do not like maxheight=none.

  1. it is a key for maximum allowed height which is stated by regulation/law in many countries.
  2. The occasion where I did find maxheight=none, thanks of this test, the tag was quite useless used under bridges.
    1. a single way had been cut into three pieces just to tag it
    2. if useful you need to tag the maximum physical height at this point.

comment:26 Changed 6 years ago by anonymous

It is widely used at least in Germany for over a year now to indicate that there's no height limit indicated via a sign - as described on the German maxheight wiki page (should probably be updated on the English version as well). Its semantics are not perfect, see the discussion on the forum http://forum.openstreetmap.org/viewtopic.php?pid=289799#p289799 posts 620-624. I agree that breaking up a way into 3 pieces to add maxheight=none is not recommended practice. Instead, the whole way should be tagged with maxheight=none (without breaking it up) where it applies (ways under bridges, tunnels, etc.). This tag is also in active use by a QA tool (http://wiki.openstreetmap.org/wiki/Maxheight_Map)

comment:27 Changed 6 years ago by Don-vip

Cc: RicoZ added

comment:28 Changed 6 years ago by Don-vip

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

comment:29 in reply to:  26 Changed 6 years ago by skyper

Replying to anonymous:

It is widely used at least in Germany for over a year now to indicate that there's no height limit indicated via a sign - as described on the German maxheight wiki page (should probably be updated on the English version as well). Its semantics are not perfect, see the discussion on the forum http://forum.openstreetmap.org/viewtopic.php?pid=289799#p289799 posts 620-624. I agree that breaking up a way into 3 pieces to add maxheight=none is not recommended practice. Instead, the whole way should be tagged with maxheight=none (without breaking it up) where it applies (ways under bridges, tunnels, etc.). This tag is also in active use by a QA tool (http://wiki.openstreetmap.org/wiki/Maxheight_Map)

The proper way in Germany to tag this would be:
maxheight=4
source:maxheight=no sign, DE:law or similar

but not maxheight=none

maxspeed=none might be OK but in Germany only on some parts of motorways and I do not know any other country without a general speed limit.

comment:30 Changed 6 years ago by mmd

As I'm not really in the position to comment on your proposal, would you mind posting your suggestion in the forum thread I mentioned above? Let's have other mappers their say on your idea. English is of course ok, never mind the German subforum.

If other mappers feel positive about your recommended tagging schema we can still work out a migration strategy for the existing maxheight=none tags if needed (details are probably beyond the scope of this josm ticket). Until we reach a (new) consensus I'd be happy to leave the current tagging as is to avoid further confusion to mappers. Thank you!

comment:31 Changed 6 years ago by mmd

Another issue: JOSM also complains about maxheight=3.5;3.9;3.5 - this is used to denote several maxheight street signs under round arches (partial maxheight values).

comment:32 in reply to:  31 ; Changed 6 years ago by EvanE

Replying to mmd:

Another issue: JOSM also complains about maxheight=3.5;3.9;3.5 - this is used to denote several maxheight street signs under round arches (partial maxheight values).

I used maxheight:left=3.5 + maxheight=3.9 + maxheight:right=3.5 in a similar case.
This of course ignores problems with the arrangement of lanes, as does the other way to express things.

comment:33 Changed 6 years ago by EvanE

With all legal limitation we have to face the fact, that ommitting a limit doesn't express "There is no limit" but at best "The value is unknown, maybe it could be unlimited".

Making OSM fit for heavy good vehicle routing more of those unlimited values may occure. I do not stick to the word "none" (as I suppose most other will not insist on the wording), you/others might prefer the word "unlimited". The word "none" was choosen in accordance to the well established maxspeed=none.

Anyway there must be a way to indicate, that a way is not limited through legal restriction. maxheight=4 doesn't make that clear, rather it implies that there is a legal limit.

In general at least two non-numeric values should be accepted for legal limitations: "unknown" and "none"/"unlimited". At least with maxspeed exist a third value "signals" indicating that maxspeed ist signaled with varying values according to traffic situation. The value "signals" may extend to other legal limitations, though this is not very probable.

comment:34 in reply to:  32 Changed 6 years ago by mmd

Replying to EvanE:

Replying to mmd:

Another issue: JOSM also complains about maxheight=3.5;3.9;3.5 - this is used to denote several maxheight street signs under round arches (partial maxheight values).

I used maxheight:left=3.5 + maxheight=3.9 + maxheight:right=3.5 in a similar case.
This of course ignores problems with the arrangement of lanes, as does the other way to express things.

In my case this applies to a single lane, I.e. a partial maxheight value. How would you tag this situation?

Changed 6 years ago by simon04

Attachment: 8687_test.patch added

comment:35 Changed 6 years ago by simon04

An idea how to embed tests directly in tagchecker.cfg: 8687_test.patch

comment:36 Changed 6 years ago by Don-vip

Resolution: fixed
Status: newclosed

In 6532/josm:

fix #8687, see #9414, see #9470 - tagchecker: update numeric tests to new MapCSS format, with embedded unit tests. MapCSS syntax updated a bit for regex.

comment:37 Changed 6 years ago by mmd

Resolution: fixed
Status: closedreopened

Re-opening ticket as skyper's proposal in Post 29 was not accepted in forum discussions.

Currently there's a strong trend towards maxheight=default instead of maxheight=none. This value is still not numeric and would trigger a JOSM warning, which is confusing mappers and lead them to delete these entries.

Some exceptions to "maxheight has to be numeric at all times" are really needed.

comment:38 Changed 6 years ago by simon04

Summary: [PATCH - unit test needs rework] Warn about letters in number only valuesWarn about letters in number only values

Since there won't be a tested release before mid of January (DevelopersGuide/Schedule), I'd suggest to await some clearer results of the discussion.

@Vincent: It would be interesting to have versions like 2014-01 to manage tickets for a specific tested release. What do you think?

comment:39 Changed 6 years ago by mmd

@simon04: if it would be easier to manage from a coding/release perspective, I would be happy to create a new ticket and close this one. What do you think?

comment:40 Changed 6 years ago by simon04

@mmd: Hmm, hard to give a general rule here. In general I prefer a new ticket (in contrast to reopen) if the old ticket is already very long and/or the required change is only loosely related to the old ticket (think of a typo vs. a wrong implementation w.r.t. the intended feature in the old ticket). … Just my thoughts …

Let's keep it as it is for now. :-)

comment:41 in reply to:  38 Changed 6 years ago by Don-vip

Replying to simon04:

@Vincent: It would be interesting to have versions like 2014-01 to manage tickets for a specific tested release. What do you think?

Let's try. I have initialized 6 milestones "14.01" to "14.06". For now only authenticated users can see them, I can change that later if needed.

comment:42 Changed 6 years ago by simon04

Milestone: 14.01

Thanks :-)

The (only) open task is comment:37.

comment:43 Changed 6 years ago by Don-vip

This should make our ticket policy clearer for closed tickets:

  • if the milestone has not been reached yet, feel free to reopen if a problem still applies.
  • if the milestone has been reached, please create a new ticket.

We should also review our "versions" system. The current list (tested/latest) is useless as it means nothing. I think we should add a new entry for each tested version, and keep a single "dev" version where the real version has to be described in ticket description.

comment:44 Changed 6 years ago by Don-vip

Well, as the situation looks very unclear, I'd vote to close the ticket as it. If something clearly emerges from @tagging, we can add exceptions through another ticket, later.

comment:45 Changed 6 years ago by simon04

Resolution: fixed
Status: reopenedclosed

Agreed. :-)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.