Provide method retain only relevant tags

[westnordost]

Fixes #11

Here, a late-vacation-night addition but still with tests and all. It would be great if you could test and review this to make sure not too many tags are dropped, @karussell . Is this API what you imagined?

[karussell]

Hey, thanks a lot! Really appreciated. Will test it soonish :)

( Currently fighting with some special US Tiger sauce. )

[karussell]

It seems to work and is faster and much less memory-intense than without.

But unfortunately with this method our German-wide import (still using our cache) is nearly 10% slower than with our current filtering method so that we would keep using ours.

But I'm also a bit unsure if our current filter can be replicated in a generic fashion that is good for a library because we:

• don't need the relationFilter
• we use a set only (no regexp) and are okay with what we would loose (I guess this is the expensive stuff)
• we have a minor custom requirement that we have to keep our internal country tag. (and the retain method removes it so that we have to re-add it)
• we skip (expensive?) conditional tags because we don't need them
• Another problem is that our tags Map should not be modified and so we have to duplicate the map to call retainOnlyRelevantTags and retainAll is called which again modifies the map. Our filter method just modifies the map once when it is filled.

[westnordost]

German-wide import. Well. If you filter out more tags than used, you are going to get incorrect results.

we have a minor custom requirement that we have to keep our internal country tag. (and the retain method removes it so that we have to re-add it)

What do you mean?

Another problem is that our tags Map should not be modified and so we have to duplicate the map to call retainOnlyRelevantTags and retainAll is called which again modifies the map. Our filter method just modifies the map once when it is filled.

Ah, I thought you needed it that way. I think I will change the API, then.

[westnordost]

we use a set only (no regexp) and are okay with what we would loose (I guess this is the expensive stuff)

That would be logical. However, currently there are no regexes. With the current data from the wiki page, we got

relevantKeyStrings = [HFCS, abutters, bicycle_road, bridge, cyclestreet, designation, dual_carriageway, expressway, frontage_road, hazard, highway, junction, lane_markings, lanes, lit, living_street, maxspeed, maxspeed:type, maxspeed:variable, motorroad, name, network, oneway, playground_zone, ref, restriction, route, rural, school_zone, service, shoulder, shoulder:both, shoulder:left, shoulder:right, side_road, sidewalk, sidewalk:both, sidewalk:left, sidewalk:right, silver_zone, source:maxspeed, surface, tracktype, tunnel, type, width, zone:maxspeed, zone:traffic]

relevantRegexes = []

I assume the extra time is due to the copying of the maps.

[karussell]

If you filter out more tags than used, you are going to get incorrect results.

Yes, this might happen. Still many things that your library supports isn't (yet) supported by GraphHopper like temporary speed restrictions or conditional stuff like "wet" and more. I mean the current state is much worse than what we get with your library plus even only the highway tag :D (and all additional precision is on top of this already much better state :) )

What do you mean?

We have to use a cache and this should be dependent on the tags Map and also on the country. As we store the country information in the tags map already it will be filtered from the retain method and we have to add it after the method is called which is some (very minor) overhead.

However, currently there are no regexes.

Oh, ok. Strange. I'll investigate then a bit.

[westnordost]

I changed the interface, it should be more flexible now plus you should be able to put your country exception right in there.

[karussell]

Oh, wow. Thank you. Will try!

[westnordost]

by the way, slightly off topic, I think it would be very important for graphhopper to mark road segments with maxspeeds inferred via implicit speed limits in one way or another (e.g. add a tag like implicit=yes?) because turn by turn navigation software should certainly not show a speed limit sign in their UI for road sections that are (assumed to have) default speed limits.

[karussell]

because turn by turn navigation software should certainly not show a speed limit sign in their UI for road sections that are (assumed to have) default speed limits.

Why not? I would not find it problematic if the software warns me about my too high speed.

But will think about it.

[westnordost]

Warning against too high speed limit is fine. However, only because it is not tagged in OSM does not mean that in reality there is no sign (with a lower limit). Showing the inferred implicit speed limit in the style of a speed limit sign would wrongly suggests that there is a speed limit sign which maybe says something else than the actual speed limit sign.

Also, note that the default speed limits are just being inferred from the data that is there. Especially with fuzzy matching, it is just a (good) guess. For example, the entry points of a rural roundabout are often oneway=yes because each side of the island are mapped as one way. The fuzzy match for this situation would be that it is a dual carriageway (unless dual_carriageway=no is set), meaning e.g. in Germany, that very short section of road would be inferred (fuzzily) to be alike to a motorway (no speed limit). That doesn't really matter for the overall route calculation, but would be somewhat confusing when shown in the UI as if this is signed.

[karussell]

Nice, this API change seems to solve the additional overhead I was seeing :) !!

Thanks a lot!

[westnordost]

Alright, so I shall merge this.