Added lanes:forward, lanes:backward and lanes:both_ways to default tags

[henrik-wolf]

As discussed in issue #86, I added the parsing for the tags lanes:forward, lanes:backward and lanes:both_ways, to provide a clearer picture of the number of lanes actually present, without breaking the old behaviour of the lanes tag.

I added documentation as to how we parse all the default tags (this probably needs some love... I feel it might be clearer, if we just copy the whole source of every parsing function into the docs)

In addition, I added some code to used the new tags during weight adding, as described in point 2.
Let me know what you think about it.

[codecov]

Codecov Report

Base: 80.08% // Head: 80.18% // Increases project coverage by +0.09% 🎉

Coverage data is based on head (8034adc) compared to base (33898b9).
Patch coverage: 88.23% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
+ Coverage   80.08%   80.18%   +0.09%     
==========================================
  Files          14       14              
  Lines        1130     1176      +46     
==========================================
+ Hits          905      943      +38     
- Misses        225      233       +8     

Impacted Files	Coverage Δ
src/parse.jl	88.68% <87.17%> (-0.15%)	⬇️
src/graph.jl	87.44% <90.00%> (-1.35%)	⬇️
src/constants.jl	92.85% <100.00%> (+0.54%)	⬆️
src/types.jl	63.33% <0.00%> (+4.07%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[jarodlam]

Thanks @SuperGrobi, this looks great! Thank you especially for updating our documentation.

I've added a few comments related to clarify how we should parse the lane numbering. Essentially, I think we should:

• Preserve OSM's meaning of the lanes tag in LightOSM's lanes tag. This keeps things consistent with OSM.
• Do our best guess at calculating lanes:forward and lanes:backward from lanes and set defaults that are consistent between all of these tags.

Let me know if you have any questions.

[henrik-wolf]

Ok. So you effectively want to double the numbers in DEFAULT_LANES? From the discussion in issue #86, I was under the impression that you wanted to keep the current lanes behaviour, due to backwards compatibility. But, given that I am also calculating the efficiency from the new lanes, there is not much backwards compatibility left to preserve... I can fix that.

What do you mean by

calculating lanes:forward and lanes:backward from lanes

? Do you want to ignore the tags of lanes:forward and lanes:backward (and lanes:both_ways) whenever given and just populate them from lanes? (e.g. use half of that?) I would not want to lose the information whenever it is available...

Otherwise, I would guess that what I am currently doing is the best way of guessing the the values. We could talk about adding a data quality warning, if the values are inconsistent.. (e.g. if lanes != lanes:forward + lanes:backward + lanes:both_ways)

Did you add comments directly to the code in "Files changed"? If so, I am for some reason not seeing them.

[jarodlam]

Ok. So you effectively want to double the numbers in DEFAULT_LANES? From the discussion in issue #86, I was under the impression that you wanted to keep the current lanes behaviour, due to backwards compatibility. But, given that I am also calculating the efficiency from the new lanes, there is not much backwards compatibility left to preserve... I can fix that.

Yes. I see this as more of a bug fix. We've been treating the lanes tag wrong, so this fixes it to align with OSM best practices.

What do you mean by

calculating lanes:forward and lanes:backward from lanes

? Do you want to ignore the tags of lanes:forward and lanes:backward (and lanes:both_ways) whenever given and just populate them from lanes? (e.g. use half of that?) I would not want to lose the information whenever it is available...

Otherwise, I would guess that what I am currently doing is the best way of guessing the the values. We could talk about adding a data quality warning, if the values are inconsistent.. (e.g. if lanes != lanes:forward + lanes:backward + lanes:both_ways)

No, we only want to auto-fill them when these tags don't exist or contain invalid information. Maybe I am mis-reading your code, and this is already what you are doing?

Did you add comments directly to the code in "Files changed"? If so, I am for some reason not seeing them.

Yes I did, I'm not sure why you can't see them 🤔? They are visible to me like this:

[henrik-wolf]

I must have misunderstood you then and will update the lanes tag today.

No, we only want to auto-fill them when these tags don't exist or contain invalid information.

I think I am already do it this way. But let me check again.

Since I have not yet done any review on pull requests, I cant really say why. A quick google search suggested that you somehow need to finish the review process? The "pending" tag on your commits might suggest that they are not visible...

[henrik-wolf]

I just reworked the parsing function in the hope of making the values we parse more consistent. I think that worked out, but at the very high price of just guessing and adding data in increasingly complicated if-else clauses. I believe the main culprit for this complexity being the default values we use whenever there is nothing available. In general, I think we can not catch any strange stuff people might have put into the tags on OSM. And every time we catch something, we again need to take an opinion... I did remove all inconsistencies arising in the test cases, but in turn, the decision I took have become so complicated, that they are not easily described in the documentation. This seems to me the latest point, at which this approach is no longer feasible.

The longer I think about it, the more I feel as it would be more sincere and beneficial for any scientific application of trying to parse the tags, and if they are not there, just put a missing. That way we get rid of as many decisions as possible, and the user can decide for himself how he wants these tags to be treated. Additional, the user actually gets to see which values have been in the data, and which would have been added by us.

I get that there are a few steps down the line which somehow depend on the the edges having some tags, but I really dislike the fact, that in the end there is no real way to figure out what we just grabbed out of thin air and what was there originally.

We could provide either a keyword like add_missing_values=true to the loader functions, or, split the generation of the graph in two steps. Where you get a second function like fill_missing_tags(graph) (or something like this, where we then can do all the stuff which actually needs all ways to have some tags.

Either way, the stuff I have been doing here does not feel like it is going anywhere...

In addition, we should decide how to we want to use "both_ways" streets in everything that involves :lane_efficiency