Modify

Opened 3 years ago

Closed 10 months ago

Last modified 10 months ago

#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 Klumbumbus)

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  
    119119/* {0.tag} without {1.key} (warning level) */
    120120node[emergency=fire_hydrant][!fire_hydrant:type],
    121121way[     boundary=administrative][!admin_level],
     122way[piste:type=nordic][!piste:grooming][!piste:difficulty],
     123way[piste:type=downhill][!piste:difficulty],
    122124relation[boundary=administrative][!admin_level],
    123125relation[route=bicycle ][!network][type=route],
    124126relation[route=hiking  ][!network][type=route],

Attachments (2)

combinations.mapcss.patch (548 bytes) - added by yvecai 3 years ago.
patch file
josm_17606.patch (2.8 KB) - added by skyper 10 months ago.
new patch including all changes of the ticket

Download all attachments as: .zip

Change History (32)

Changed 3 years ago by yvecai

Attachment: combinations.mapcss.patch added

patch file

comment:1 Changed 3 years ago by Klumbumbus

Description: modified (diff)
Type: defectenhancement

comment:2 Changed 16 months ago by yvecai

What can be done to get this into consideration?

comment:3 Changed 16 months ago by skyper

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.

comment:4 Changed 16 months ago by 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
  • ...

comment:5 in reply to:  3 ; Changed 14 months ago by anonymous

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 in reply to:  4 Changed 14 months ago by anonymous

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 in reply to:  5 Changed 14 months ago by skyper

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();
}
Last edited 14 months ago by skyper (previous) (diff)

comment:8 Changed 14 months ago by anonymous

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 Changed 12 months ago by yvecai

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 Changed 12 months ago by yvecai

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 Changed 12 months ago by skyper

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 Changed 12 months ago by yvecai

@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 Changed 12 months ago by skyper

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 Changed 12 months ago by yvecai

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.

comment:15 Changed 12 months ago by skyper

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?

comment:16 Changed 12 months ago by yvecai

Hi,
I tested it, this looks fine to me, good job !

comment:17 Changed 12 months ago by yvecai

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 in reply to:  17 Changed 12 months ago by skyper

Replying to yvecai:

Hi,
I tested it, this looks fine to me, good job !

Thanks, yes, I tested this with real data.

Replying to yvecai:

Just correcting a spelling mistake:

Nice catch.

comment:19 Changed 11 months ago by skyper

Milestone: 21.06

comment:20 in reply to:  15 ; Changed 11 months ago by 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. Why did you even add these two pseudo classes?

comment:21 in reply to:  20 ; Changed 11 months ago by skyper

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.

comment:22 in reply to:  21 ; Changed 11 months ago by Klumbumbus

And why the :new? to distinct the severity (warning/other) between new and old objects?

comment:23 in reply to:  22 Changed 11 months ago by skyper

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.

Last edited 11 months ago by skyper (previous) (diff)

comment:24 Changed 11 months ago by Don-vip

Milestone: 21.0621.07

Changed 10 months ago by skyper

Attachment: josm_17606.patch added

new patch including all changes of the ticket

comment:25 Changed 10 months ago by skyper

I've attached a "final" patch file (josm_17606.patch) including all above and the missing !:new.

comment:26 Changed 10 months ago by Don-vip

Resolution: fixed
Status: newclosed

In 18047/josm:

fix #17606 - piste:type validation improvement (patch by skyper)

comment:27 Changed 10 months ago by Don-vip

thanks!

comment:28 Changed 10 months ago by skyper

You are welcome. I try to support JOSM as much as I can with my limited skills.

comment:29 Changed 10 months ago by Don-vip

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.

comment:30 Changed 10 months ago by Don-vip

In 18050/josm:

see #17606 - see #21126 - fixes

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.