#17606 closed enhancement (fixed)
[Patch] piste:type validation improvement
| Reported by: | yvecai | Owned by: | team |
|---|---|---|---|
| Priority: | normal | Milestone: | 21.07 |
| Component: | Core validator | Version: | |
| Keywords: | piste, ski | Cc: |
Description (last modified by )
While piste:difficulty is more than just useful for the data user, piste:grooming for crosscountry skiing appears to be defaults to quite different practises across the world.
-
combinations.mapcss
old new 119 119 /* {0.tag} without {1.key} (warning level) */ 120 120 node[emergency=fire_hydrant][!fire_hydrant:type], 121 121 way[ boundary=administrative][!admin_level], 122 way[piste:type=nordic][!piste:grooming][!piste:difficulty], 123 way[piste:type=downhill][!piste:difficulty], 122 124 relation[boundary=administrative][!admin_level], 123 125 relation[route=bicycle ][!network][type=route], 124 126 relation[route=hiking ][!network][type=route],
Attachments (2)
Change History (32)
by , 5 years ago
| Attachment: | combinations.mapcss.patch added |
|---|
comment:1 by , 5 years ago
| Description: | modified (diff) |
|---|---|
| Type: | defect → enhancement |
follow-up: 5 comment:3 by , 3 years ago
| Summary: | piste:type validation improvement → [Patch] piste:type validation improvement |
|---|
Add [Patch] to the summary. Ping, like you did.
Mmh, I usually tag the piste:difficulty in the piste's route relation and not on each individual way. As I seldom have proper data for the correct incline of each small part I wonder how to tag each way correctly, especially in areas with forest.
I would include skitour, additionally.
follow-up: 6 comment:4 by , 3 years ago
I'm not sure if the patch does what you actually want. Please describe in words which warnings you want, e.g.
- piste:type=nordic without piste:grooming
- ...
follow-up: 7 comment:5 by , 3 years ago
Replying to skyper:
Add
[Patch]to the summary. Ping, like you did.
Mmh, I usually tag the piste:difficulty in the piste's route relation and not on each individual way. As I seldom have proper data for the correct incline of each small part I wonder how to tag each way correctly, especially in areas with forest.
I would include
skitour, additionally.
@Skyper: Actually, the easiest way to gather data about difficulty is with your skis ;-)
comment:6 by , 3 years ago
Replying to Klumbumbus:
I'm not sure if the patch does what you actually want. Please describe in words which warnings you want, e.g.
- piste:type=nordic without piste:grooming
- ...
@Klumbubus The goal is :
- to give a warning at validation if piste:type=nordic is present, but not piste:grooming=* nor piste:difficulty=*.
- to give a warning at validation if piste:type=downhill is present, but not piste:difficulty=*.
comment:7 by , 3 years ago
Replying to anonymous:
Replying to skyper:
Mmh, I usually tag the piste:difficulty in the piste's route relation and not on each individual way. As I seldom have proper data for the correct incline of each small part I wonder how to tag each way correctly, especially in areas with forest.
I would include
skitour, additionally.
@Skyper: Actually, the easiest way to gather data about difficulty is with your skis ;-)
Yes, that is what I only do. Still I only have poor sources as GPS has problems with forest and in a country side with steeper inclines. The weather is not that stable which is a problem for ele measuring and I often forget about it or even do not find a source to calibrate my unit.
I usually do not stop every few meters to take pictures or make some written notes as I like to find my rhythm and have often no time as I want to make some distance.
Long story short, I think the quality of difficulty will be quite low if we start to tag it on every small way a piste uses. So, in my eyes, I stay with the relation and a general classification and would ignore this warning, most of the times. On the other hand, I wonder why relations are not checked at all.
I would prefer other instead of warning level for the difficulty tag on ways for nordic. Grooming is more important, e.g. check both tags separately, and include relations where I do not have a problem with warning level for both tags
way[piste:type =~ /^downhill|nordic|skitour$/][!piste:grooming], relation[piste:type =~ /^downhill|nordic|skitour$/][!piste:grooming][type=piste], relation[piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty][type=piste] { throwWarning(); } way[piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty] { throwOther(); }
comment:8 by , 3 years ago
Hello Skyper,
I agree with you concerning nordic ways grooming.
Downhill grooming is however documented as implicit unless stated otherwise. Maybe could be stated as such also for Skitour, as piste:grooming = classic seems to be rather exceptional and was documented only recently on the wiki.
For nordic difficulty, your practice is just the opposite as mine, so I guess this is not the good place to discuss.
About relations, your validation rules would be contradictory to the wiki documentation on route=piste. Incidentally I open a discussion this morning on the subject, so you may want to follow up here: https://wiki.openstreetmap.org/wiki/Talk:Key:piste:difficulty
I think it's good practice to align validation to the documentation, so let's discuss the subjects first and come back to this issue later on.
Regards, Yves
comment:9 by , 3 years ago
I propose the following for piste:type=nordic, as both difficulty and grooming are set on either ways or relation :
For a way piste:type=nordic
- warn if piste:grooming is not set on the way neither a route relation piste:type=nordic this way is a member
- other if piste:difficulty is not set on the way neither a route relation piste:type=nordic this way is a member
Though I'm not sure how to check for the relation membership trough the validation rules. Is it a built-in feature ?
comment:10 by , 3 years ago
I did not found the proper doc page this morning, now I do:
Code highlighting:
way[piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty] < relation[route =~ /^piste|ski$/][piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty] { throwWarning: tr("No difficulty defined for skiing route nor its members"); } relation[route =~ /^piste|ski$/][piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty] > way[piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty] { throwWarning: tr("No difficulty defined for skiing way nor a parent skiing route"); } way[piste:type =~ /^nordic$/][!piste:grooming] < relation[route =~ /^piste|ski$/][piste:type =~ /^nordic$/][!piste:grooming] { throwWarning: tr("No grooming defined for nordic skiing route nor its members"); } relation[route =~ /^piste|ski$/][piste:type =~ /^nordic$/][!piste:grooming] > way[piste:type =~ /^nordic$/][!piste:grooming]{ throwWarning: tr("No grooming defined for nordic skiing way nor a parent skiing route"); }
This work as intended where ways are member of relations. This is pretty verbose and the missing tags are spotted twice (once on the ways, once on the relations). I think that most accurate mapping can be obtained in mapping grooming and difficulty on ways rather than relation, so my preference would go for a simpler:
Code highlighting:
relation[route =~ /^piste|ski$/][piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty] > way[piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty] { throwWarning: tr("No difficulty defined for skiing [...]"); } relation[route =~ /^piste|ski$/][piste:type =~ /^nordic$/][!piste:grooming] > way[piste:type =~ /^nordic$/][!piste:grooming]{ throwWarning: tr("No grooming defined for nordic skiing [...]"); }
The problem is that there is apparently no way to consider separately pistes mapped as simple ways, you can't do:
Code highlighting:
!relation[route =~ /^piste|ski$/] < way[piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty] { throwOther: tr("No difficulty defined on ski way");
comment:11 by , 3 years ago
Take a look at the eval expressions parent_tag() and parent_tags(). I think most of the test can be written without the child/parent selector.
comment:12 by , 3 years ago
@skyper, thank you, it works like a charm !
Code highlighting:
way[piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty][count(parent_tags("piste:difficulty")) == 0] { throwWarning: tr("No difficulty defined for skiing"); } way[piste:type =~ /^nordic$/][!piste:grooming][count(parent_tags("piste:grooming")) == 0] { throwWarning: tr("No grooming defined for nordic skiing"); } relation[piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty] > way[!piste:difficulty] { throwWarning: tr("No difficulty defined for skiing"); } relation[piste:type =~ /^nordic$/][!piste:grooming] > way[!piste:grooming]{ throwWarning: tr("No grooming defined for nordic skiing"); }
The first two rules check warn if the tag is neither on the way nor on an hypothetical parent relation.
The last two are relaxed enough to warn if the tag is not on the relation build with ways wihout any tags.
That should cover every mapping habits, I think.
comment:13 by , 3 years ago
I need to take a closer look but I think we need to exclude incomplete members and a check for download area for the test with parent_tags.
Did you test this with a way with memberships of several route relations but only some with the subtags?
comment:14 by , 3 years ago
Did you test this with a way with memberships of several route relations but only some with the subtags?
Just did. Say a way (with or without piste:type, but without diffculty set one the way) is member of two routes, one with the difficulty tag set, one without. Then the fact the way is member of a route without difficulty set will trigger a warning despite the difficulty of the other route is set.
At least this shows inconsistency in tagging, so in my opinion the outcome is correct.
Keep in mind that we are validating two different mapping habits here. We can't really predict from incomplete tagging if the mapper intended to map difficulty (resp. grooming) on ways or relations, so I deliberately choose to validate the ways.
Ways being the simplest elements, they are easier to 'fix'. I also guess that the 'route' mappers will know better what they are doing.
For incomplete members, I'm not sure what is the common validation policy. But again, as the ways are validated here, no warning is issued on relations, complete or not.
follow-up: 20 comment:15 by , 3 years ago
There was a problem with download area as I expected and one with duplicate warnings caused by both methods (child selector <-> parent_tag).
- As I have written above I am fine with a general test for grooming.
- At least informal warnings can be issued in all cases.
I changed it accordingly plus added the group and test cases. Please try:
/* piste subtags, see #17606 */ way[piste:type=nordic][!piste:grooming] { throwWarning: tr("No grooming defined for {0} skiing, add {1}", "nordic", "piste:grooming="); group: tr("missing tag"); assertMatch: "way piste:type=nordic"; assertNoMatch: "way piste:type=nordic piste:grooming=classic"; } way[piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty][count(parent_tags("piste:difficulty")) == 0] { set missing_parent_piste_diffility; } way.missing_parent_piste_diffility:new, way.missing_parent_piste_diffility:in-downloaded-area { throwWarning: tr("No difficulty defined for skiing, add {0}", "piste:difficulty="); group: tr("missing tag"); set missing_piste_diffility; assertMatch: "way piste:type=nordic (count(parent_tag("piste:difficulty")) == 0)"; assertNoMatch: "way piste:type=nordic (count(parent_tag("piste:difficulty")) == 1)"; assertNoMatch: "way piste:type=nordic piste:difficulty=easy"; } relation[piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty] > way[!piste:difficulty]!.missing_piste_diffility { throwWarning: tr("No difficulty defined for skiing, add {0}", "piste:difficulty="); group: tr("missing tag"); set missing_piste_diffility_both; assertMatch: "way piste:type=nordic (count(parent_tag("piste:difficulty")) == 0)"; assertNoMatch: "way piste:type=nordic (count(parent_tag("piste:difficulty")) == 1)"; assertNoMatch: "way piste:type=nordic piste:difficulty=easy"; } way[piste:type=nordic][!piste:difficulty]!.missing_piste_diffility!.missing_piste_diffility_both { throwOther: tr("No difficulty defined for skiing, add {0}", "piste:difficulty="); group: tr("missing tag"); assertMatch: "way piste:type=nordic"; assertNoMatch: "way piste:type=nordic piste:difficulty=easy"; } relation[piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty] { throwOther: tr("No difficulty defined for skiing, add {0}", "piste:difficulty="); group: tr("missing tag"); assertMatch: "relation piste:type=nordic"; assertNoMatch: "relation piste:type=nordic piste:difficulty=easy"; } relation[piste:type=nordic][!piste:grooming]{ throwOther: tr("No grooming defined for {0} skiing, add {1}", "nordic", "piste:grooming="); group: tr("missing tag"); assertMatch: "relation piste:type=nordic"; assertNoMatch: "relation piste:type=nordic piste:grooming=classic"; }
@team:
Should be added to combinations.mapcss.
Is this enough or do you want a patch file?
follow-up: 18 comment:17 by , 3 years ago
Just correcting a spelling mistake:
/* piste subtags, see #17606 */ way[piste:type=nordic][!piste:grooming] { throwWarning: tr("No grooming defined for {0} skiing, add {1}", "nordic", "piste:grooming="); group: tr("missing tag"); assertMatch: "way piste:type=nordic"; assertNoMatch: "way piste:type=nordic piste:grooming=classic"; } way[piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty][count(parent_tags("piste:difficulty")) == 0] { set missing_parent_piste_difficulty; } way.missing_parent_piste_difficulty:new, way.missing_parent_piste_difficulty:in-downloaded-area { throwWarning: tr("No difficulty defined for skiing, add {0}", "piste:difficulty="); group: tr("missing tag"); set missing_piste_difficulty; assertMatch: "way piste:type=nordic (count(parent_tag("piste:difficulty")) == 0)"; assertNoMatch: "way piste:type=nordic (count(parent_tag("piste:difficulty")) == 1)"; assertNoMatch: "way piste:type=nordic piste:difficulty=easy"; } relation[piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty] > way[!piste:difficulty]!.missing_piste_difficulty { throwWarning: tr("No difficulty defined for skiing, add {0}", "piste:difficulty="); group: tr("missing tag"); set missing_piste_difficulty_both; assertMatch: "way piste:type=nordic (count(parent_tag("piste:difficulty")) == 0)"; assertNoMatch: "way piste:type=nordic (count(parent_tag("piste:difficulty")) == 1)"; assertNoMatch: "way piste:type=nordic piste:difficulty=easy"; } way[piste:type=nordic][!piste:difficulty]!.missing_piste_difficulty!.missing_piste_difficulty_both { throwOther: tr("No difficulty defined for skiing, add {0}", "piste:difficulty="); group: tr("missing tag"); assertMatch: "way piste:type=nordic"; assertNoMatch: "way piste:type=nordic piste:difficulty=easy"; } relation[piste:type =~ /^downhill|nordic|skitour$/][!piste:difficulty] { throwOther: tr("No difficulty defined for skiing, add {0}", "piste:difficulty="); group: tr("missing tag"); assertMatch: "relation piste:type=nordic"; assertNoMatch: "relation piste:type=nordic piste:difficulty=easy"; } relation[piste:type=nordic][!piste:grooming]{ throwOther: tr("No grooming defined for {0} skiing, add {1}", "nordic", "piste:grooming="); group: tr("missing tag"); assertMatch: "relation piste:type=nordic"; assertNoMatch: "relation piste:type=nordic piste:grooming=classic"; }
comment:18 by , 3 years ago
comment:19 by , 3 years ago
| Milestone: | → 21.06 |
|---|
follow-up: 21 comment:20 by , 3 years ago
Replying to skyper:
way.missing_parent_piste_difficulty:new, way.missing_parent_piste_difficulty:in-downloaded-area {
This creates double warnings on the same way. Why did you even add these two pseudo classes?
follow-up: 22 comment:21 by , 3 years ago
Replying to Klumbumbus:
Replying to skyper:
way.missing_parent_piste_difficulty:new, way.missing_parent_piste_difficulty:in-downloaded-area {This creates double warnings on the same way.
Ok, a !:new needes to be added
way.missing_parent_piste_difficulty:new, way.missing_parent_piste_difficulty:in-downloaded-area!:new {
Why did you even add these two pseudo classes?
If outside download area or without download area parent relations might not be downloaded and therefor [count(parent_tags("piste:difficulty")) == 0] is always true.
follow-up: 23 comment:22 by , 3 years ago
And why the :new? to distinct the severity (warning/other) between new and old objects?
comment:23 by , 3 years ago
Replying to Klumbumbus:
And why the
:new? to distinct the severity (warning/other) between new and old objects?
Cause splitting can lead to new ways outside download area with hopefully all parent relations downloaded area and adding new routes without any download area should trigger the higher severity.
comment:24 by , 3 years ago
| Milestone: | 21.06 → 21.07 |
|---|
comment:25 by , 3 years ago
I've attached a "final" patch file (josm_17606.patch) including all above and the missing !:new.
comment:28 by , 3 years ago
You are welcome. I try to support JOSM as much as I can with my limited skills.
comment:29 by , 3 years ago
Tagging requires a lot of time I don't have. So help is very welcome. Right now the tagging priority would be for me #20835 so that we get integration tests OK again on Jenkins.
patch file