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

Advisory speed limit added #4870

Merged
merged 16 commits into from
Mar 15, 2018
Merged

Advisory speed limit added #4870

merged 16 commits into from
Mar 15, 2018

Conversation

umarpreet1
Copy link
Contributor

i created advisory speed limit field and added it to preset highway=road and highway=motorway #4522

@@ -5,6 +5,7 @@
"ref_road_number",
"oneway_yes",
"maxspeed",
"maxspeed_advisory",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally advisory speed limits appear on motorway_link and other *_link roads. It can happen on non-links too, but typically only at curves.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so i have to add this field to all *link roads or something else that i should do.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would make sense to add this field to all the *_link presets. Maybe even remove maxspeed in favor of maxspeed_advisory on those presets.

The non-link presets should be consistent with each other. So if motorway has it, then the other highway presets like residential and tertiary should too. For these presets, an extra “Advisory Speed Limit” field could be confusing if it appears by default, since most roads don’t have advisory speed limits. I wonder if we should leave it off of these non-link presets until #4871 is implemented.

@bhousel
Copy link
Member

bhousel commented Mar 10, 2018

This is a great start @umarpreet1 - some quick feedback:

  • To echo what @1ec5 said - I agree these should only be on _link way presets for now.

  • We have a naming convention for colon-namespaced fields. Can you:

    • move the field definition file to data/presets/fields/maxspeed/advisory.json
    • inside that file, "key": "maxspeed:advisory"
    • when referring to the field in a preset, it should be called "maxspeed/advisory"

@slhh
Copy link
Contributor

slhh commented Mar 11, 2018

The key maxspeed:advisory is a rarely used key (12500 uses) and seems to be applicable mainly in the US and a few European countries only: https://taginfo.openstreetmap.org/keys/maxspeed%3Aadvisory#map
For comparision, the key maxspeed is used approximately 9 millon times.

@1ec5
Copy link
Collaborator

1ec5 commented Mar 11, 2018

To be sure, I wasn’t suggesting that maxspeed be removed from all the highway presets, just the *_link ones. You’re right that maxspeed:advisory is much rarer than maxspeed. After all, advisory speed limits are rarer than legal speed limits in reality:

  • 100,502 ways are tagged highway=motorway_link and maxspeed.
  • 8,588 ways are tagged highway=motorway_link and maxspeed:advisory.

Specifically in the U.S., where legal speed limits are virtually never posted on freeway ramps:

  • 11,439 ways are incorrectly tagged highway=motorway_link and maxspeed.
  • 6,977 ways are correctly tagged highway=motorway_link and maxspeed:advisory.

One reason I opened #4522 is that mappers unaware of the maxspeed:advisory tag end up putting advisory speed limits in maxspeed, even when it’s semantically incorrect. There is some risk that presenting both fields alongside each other could confuse some mappers, which is why I raised the possibility of removing maxspeed from *_link presets only. But there’s a help button in case anyone is wondering what an advisory speed limit is.

In any case, I don’t want to derail this PR with this relatively minor question. @bhousel’s suggestions in #4870 (comment) would leave maxspeed in place in all the presets, which is fine for now.

@umarpreet1
Copy link
Contributor Author

i have changed the following in as mentioned by you:-
1.changed the naming convention and file convention as stated by you.
2.removed previous advisory speed limit from motorway and road.
3. replaced max speed limit by advisory speed limit in all the *link ways in highways preset.

@slhh
Copy link
Contributor

slhh commented Mar 11, 2018

@1ec5

To be sure, I wasn’t suggesting that maxspeed be removed from all the highway presets, just the *_link ones.

Removing maxspeed from link presets would generate the inverse problem in other countries.
E.g. in Germany, limits on ramps are always legal maxspeed limits, a traffic sign to post advisory limits doesn't exist.
Even in the U.S., you might need maxspeed on ramps. Doesn't the general legal speed limit of the freeway persist, where an advisory limit is posted?

@althio
Copy link
Contributor

althio commented Mar 11, 2018

@1ec5 Maybe even remove maxspeed in favor of maxspeed_advisory on those presets
...
To be sure, I wasn’t suggesting that maxspeed be removed from all the highway presets, just the *_link ones.

@slhh Removing maxspeed from link presets would generate the inverse problem in other countries.

I agree with @slhh
Add maxspeed_advisory on *_link if it feels better for some countries, but do not remove plain maxspeed on *_link, just propose both.

@1ec5
Copy link
Collaborator

1ec5 commented Mar 11, 2018

Even in the U.S., you might need maxspeed on ramps. Doesn't the general legal speed limit of the freeway persist, where an advisory limit is posted?

As with anything in the U.S., the answer varies by state. In some states, no legal speed limit applies to on- and off-ramps. In others, the freeway’s legal speed limit technically applies. But either way, driving over the advisory speed limit could subject the driver to a citation for dangerous driving according to the basic rule, which makes the legal speed limit a moot point. (Since legal speed limits aren’t signposted on ramps in the U.S., one could argue that they aren’t verifiable on the ground and therefore should be left off of link ways.)

Removing maxspeed from link presets would generate the inverse problem in other countries.

Add maxspeed_advisory on *_link if it feels better for some countries, but do not remove plain maxspeed on *_link, just propose both.

Agreed. It was just a suggestion, and it’s clear from this discussion that maxspeed shouldn’t be removed until some of @slhh’s ideas in #4871 (comment) are implemented.

(Thanks for putting up with this back-and-forth, @umarpreet1!)

Copy link

@Nakaner Nakaner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

@@ -4,7 +4,7 @@
"name",
"ref_road_number",
"oneway",
"maxspeed",
"maxspeed/advisory",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxspeed=* is a valid tag on higway=motorway_link in some countries and should not be removed.

@@ -3,7 +3,7 @@
"fields": [
"name",
"oneway",
"maxspeed",
"maxspeed/advisory",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxspeed=* is a valid tag on higway=primary_link in some countries and should not be removed.

@@ -3,7 +3,7 @@
"fields": [
"name",
"oneway",
"maxspeed",
"maxspeed/advisory",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxspeed=* is a valid tag on higway=secondary_link in some countries and should not be removed. highway=secondary_link in settlements where the normal urban speed limit (e.g. 50 kph in many European countries) applies should get maxspeed=50 or maxspeed=<country_code>:urban.

@@ -3,7 +3,7 @@
"fields": [
"name",
"oneway",
"maxspeed",
"maxspeed/advisory",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxspeed=* is a valid tag on higway=tertiary_link in some countries and should not be removed. highway=secondary_link in settlements where the normal urban speed limit (e.g. 50 kph in many European countries) applies should get maxspeed=50 or maxspeed=<country_code>:urban.

@@ -4,7 +4,7 @@
"name",
"ref_road_number",
"oneway",
"maxspeed",
"maxspeed/advisory",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxspeed=* is a valid tag on higway=trunk_link in some countries and should not be removed.

@bhousel
Copy link
Member

bhousel commented Mar 15, 2018

Just catching up on the thread...

I'm apologize - what I wrote above in my previous comment was confusing. I meant to say that link roads should continue to have the maxspeed field, but they should also be the only roads that have the maxspeed:advisory field.

I'll cleanup and merge - thanks for your review @Nakaner @slhh @althio & @1ec5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants