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

checking for ferry terminals instead of ferry routes. #2167

Merged
merged 5 commits into from
Mar 18, 2024

Conversation

ComradeRamen
Copy link
Contributor

I started going through ferry route ends not tagged with these and will either fix them or relay back here.
Will give an update on progress tomorrow.

@ComradeRamen
Copy link
Contributor Author

#2166

@Famlam
Copy link
Collaborator

Famlam commented Mar 15, 2024

Hi,
Based on the description I suspect it's work in progress, so not reviewing the code yet, but would you expect slipways on bicycle paths, or secondary and above major roads? Slipways are for letting boats enter the water, which sounds wrong to me. (Imagine a high-priority motorway full of boat-carrying cars that all want to let their boat into the water there)

@ComradeRamen ComradeRamen marked this pull request as draft March 15, 2024 21:14
@ComradeRamen
Copy link
Contributor Author

ComradeRamen commented Mar 16, 2024

Out of 4476 connecting nodes only 138 didn't have the amenity=ferry_terminal tag which I have since fixed. From what I can tell this PR should now not throw any new issues and should also resolve any "false positives" where the ferry_terminal is mapped but not the ferry route.

@ComradeRamen ComradeRamen marked this pull request as ready for review March 16, 2024 01:11
@ComradeRamen ComradeRamen changed the title checking for ferry terminals and slipways instead of ferry routes. checking for ferry terminals instead of ferry routes. Mar 16, 2024
@ComradeRamen
Copy link
Contributor Author

ComradeRamen commented Mar 16, 2024

Full Script/s used. Overpass request takes ~3 minutes.

@Famlam
Copy link
Collaborator

Famlam commented Mar 16, 2024

Thanks! I think you can remove lines 57 to 62 and line 86

SELECT
    wid,
    nid,
    geom,
    highway
FROM (
) AS t

They don't serve a purpose anymore now that the JOIN on ferry routes is gone

@ComradeRamen
Copy link
Contributor Author

can remove lines 57 to 62 and line 86

Done.

Btw I interpreted "Highway from motorway to tertiary are important ways. They should
lead to somewhere and in particular to a network of minor roads." as meaning tertiary roads are included in the check even tho they're not.

@Famlam
Copy link
Collaborator

Famlam commented Mar 17, 2024

Btw I interpreted "Highway from motorway to tertiary are important ways. They should lead to somewhere and in particular to a network of minor roads." as meaning tertiary roads are included in the check even tho they're not.

The description is technically correct, because they should be connected, but it causes quite some issues in some less densely mapped areas for which I don't know better solutions. E.g. in sweden_gotland, for example https://www.openstreetmap.org/way/55408443 , on first glance I have the impression that the custom in this country is to map (nearly) all I-numbered roads as tertiary.
In addition, there's quite a couple of tertiaries that end in a highway=construction road which would all be flagged. It's very uncommon for an entire road to be mapped with a different classification only because there's a few months construction at a small part of the road.
Hence it's mentioned in the message that they should be connected, without actually giving a warning.

Done.

Thanks!

I'll give it a quick run on one or two countries later to be sure that there's no side effects (currently running something else...) but code-technically I don't have comments anymore.

@ComradeRamen
Copy link
Contributor Author

ComradeRamen commented Mar 17, 2024

Makes sense.

Btw why are construction roads excluded from endpoints unlike class 3.

@frodrigo frodrigo merged commit d2551e7 into osm-fr:dev Mar 18, 2024
3 checks passed
@ComradeRamen ComradeRamen deleted the dev2 branch March 18, 2024 09:47
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.

3 participants