Exclude partially downloaded, closed ways from "Should be polygon"-check #1951
Conversation
| (ways.tags?'amenity' AND ways.tags->'amenity' in ('bar', 'biergarten', 'cafe', 'fast_food', 'food_court', 'ice_cream', 'pub', 'restaurant', 'college', 'kindergarten', 'library', 'public_bookcase', 'school', 'music_school', 'driving_school', 'language_school', 'university', 'bicycle_repair_station', 'bicycle_rental', 'boat_sharing', 'bus_station', 'car_rental', 'car_sharing', 'car_wash', 'ferry_terminal', 'fuel', 'motorcycle_parking', 'parking', 'parking_space', 'taxi', 'bank', 'baby_hatch', 'clinic', 'dentist', 'doctors', 'hospital', 'nursing_home', 'pharmacy', 'social_facility', 'veterinary', 'blood_donation', 'arts_centre', 'brothel', 'casino', 'cinema', 'community_centre', 'fountain', 'gambling', 'nightclub', 'planetarium', 'social_centre', 'studio', 'swingerclub', 'theatre', 'animal_boarding', 'animal_shelter', 'courthouse', 'coworking_space', 'crematorium', 'crypt', 'dive_centre', 'dojo', 'embassy', 'fire_station', 'firepit', 'game_feeding', 'grave_yard', 'gym', 'hunting_stand', 'internet_cafe', 'kneipp_water_cure', 'marketplace', 'place_of_worship', 'police', 'post_office', 'prison', 'public_building', 'ranger_station', 'recycling', 'rescue_station', 'sauna', 'shelter', 'shower', 'toilets', 'townhall', 'waste_transfer_station')) OR | ||
| ways.tags?'building' | ||
| ) AND | ||
| ways.linestring IS NOT NULL AND | ||
| NOT ways.is_polygon AND | ||
| ways.nodes[1] != ways.nodes[array_length(ways.nodes,1)] AND -- See #1947, excludes partially downloaded closed polygons |
There was a problem hiding this comment.
Sorry. But I do not understand the trick here.
There was a problem hiding this comment.
For a (valid) polygon where some nodes aren't downloaded, is_polygon (and even ST_IsClosed) all return false, even though they would return true if the way would've been fully downloaded.
Hence, the border region in i.e. Venezuela, Brazil etc (see issue) is cluttered with false positive markers.
However, for the nodes inside the borders of the extract, checking that the first node (id) isn't the last node (id) works very well.
Although I could technically LEFT JOIN on all nodes to verify they exist, and ignore any way where a node isn't downloaded this would also remove correct detections for partially downloaded (invalid) ways. (In addition to being slower).
In addition, adding the first node != last node check removes a lot of other false positives or redundant detections. And for this analyzer it doesn't affect what's detected.
There was a problem hiding this comment.
Not sure about that. We use the enableKeepPartialLinestring osmosis option (I added it myself https://github.com/openstreetmap/osmosis/pull/121/files) to avoid break/lost linestring on border.
Not sure. But is_polygon / ST_IsClosed should be valid, I guess. Maybe the issue is here.
There was a problem hiding this comment.
Not sure where I can modify the behavior of ST_IsClosed (and others).
It would be good if we could just use ST_IsClosed for this analyser (I think that's necessary anyway, given the number of other issues it fixes)
There was a problem hiding this comment.
No, ST_IsClosed is a Postgis function.
There was a problem hiding this comment.
Hmm, hadn't thought of it being a compressed file in a compressed file :)
Works for netherlands_zeeland; testing it against others too now. I assume you don't want me to include it in this PR yet?
(The PR as it is now already fixes some issues and will be needed anyway, so IMO it can be merged while we wait for Osmosis to merge it)
There was a problem hiding this comment.
If it is ok for you. We need to make a PR on osmose to change osmosis, event before the osmosis PR was merged.
There was a problem hiding this comment.
I tested it on 13 countries. So far the results look good: this PR + the updated osmosis remove a lot of false positives. Note this was with the version which did add a second node for a 1-node-in-the-extract-way.
We need to make a PR on osmose to change osmosis, event before the osmosis PR was merged.
I'm fine with updating it in Osmose before it's merged into Osmosis (either in this PR or in a separate PR, they're both needed but this current PR is mergeable without the other). Would you be able to do this? I can't build the package myself (I think) and since the change with >0 nodes -> >1 nodes was made after you build the package, it would need to be rebuild.
There was a problem hiding this comment.
I made the Osmose Osmosis PR #1963
There was a problem hiding this comment.
Thanks! I'm testing that one now. Is there anything that I have to do here or can this one be merged? This one should be merged before the other PR (otherwise it'll give errors on every closed way with 2 nodes in the extract).
While diving into #1951 I noticed that the roller coaster exception in ImportDatabase_Prepare is wrong. attraction=roller_coaster does refer to the area where the entire roller coaster is located (see wiki: https://wiki.openstreetmap.org/wiki/Tag:attraction%3Droller_coaster ). Consequently, it must be a polygon (or a node). For the track, the tag roller_coaster=track (https://wiki.openstreetmap.org/wiki/Tag:roller_coaster%3Dtrack) has to be used. The wiki's both explicitly state that this tag should not be combined with attraction=roller_coaster. The track is of course not a polygon, even if it's a closed loop. note that this exception was added almost 12 years ago, when roller_coaster=track didn't exist yet.
Invalid ways can be either: - truly invalid (in which case there's a different analyser that gives a better warning) - the result of a partially downloaded way (in which case we shouldn't warn), example: a valid closed way with only 2 nodes in the extract gives an overlapping linestring as the 'outside the extract' nodes are missing) Also remove a bad copy-paste
…etmap/osmosis#134 osm-fr#1951 Keep closed linestring closed even if the original way is closed and missing start/end node location
…etmap/osmosis#134 #1951 Keep closed linestring closed even if the original way is closed and missing start/end node location
See #1947
This PR excludes partially downloaded, closed ways from the "should be polygon" check. This occurs quite frequently for larger landuses near the extract borders, see the issue for examples.
In addition it removes
leisure=userandleisure=defined, which don't exist in OSM and are very likely just a copy-paste mistake (because the table on the wiki ends withuser defined)On top of this, as a positive side effect, the PR also removes the very strange warnings for:
building=*with anattraction=roller_coastertag.For example: https://osmose.openstreetmap.fr/nl/map/#zoom=18&lat=51.64769&lon=5.051999&item=1170&level=1&tags=&fixable= .
It's a strange way of tagging it (which is used for 322 out of 2503 rollercoaster ways), but the warning didn't make sense as it's a polygon. (Note:
attraction=roller_coasteris whitelisted in ImportDatabase_Prepare)