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

Fix parsing of duration tag for hh:mm:ss format. #514

Merged
merged 1 commit into from
Sep 14, 2015

Conversation

ratrun
Copy link
Contributor

@ratrun ratrun commented Sep 12, 2015

According to http://wiki.openstreetmap.org/wiki/Key:duration there is no dd:hh:mm format, instead this format needs to be interpreted as hh:mm:ss

The modification also fixes issue #501

According to http://wiki.openstreetmap.org/wiki/Key:duration there is no dd:hh:mm format, instead this format needs to be interpreted as hh:mm:ss
* This method parses a string ala "00:00" (hours and minutes) or "0:00:00" (days, hours and
* minutes).
* This method parses a string ala 'hh:mm', format for hours and minutes 'mm', 'hh:mm' or 'hh:mm:ss'
* FIXME: Add support for ISO_8601
Copy link
Member

Choose a reason for hiding this comment

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

The problem is not that it can't be added. The problem are several wrongly mapped duration tags. I think that it was like mixing minute stuff with month, although it is stated in the wiki: duration=PT71M - A duration of 71 minutes. This is not the same as P71M which denotes 71 months.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the meantime I have added support for ISO_8601 and intended to add it as separte pull request. I don't think that it is a good idea to just say that it is often filled out in a wrong way and even document in a comment a example of way with a wrong usage. First I would fix this way.
Shouldn't it be possible to add a simple plausability check based on the distance, when it comes to the decision if to consider the value of the duration tag? In case that the calulated speed is less then some value e.g. 0.1 km/h we could ignore it and use the calculated duration based on an the assumed ferry speed instead.

Copy link
Member

Choose a reason for hiding this comment

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

100% agree - nice idea! Thanks & looking forward to the PR!

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can log these mismatches and create a blog post with these entries so that we or mappers could fix them (e.g. creating OSM notes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea with the logging and the blogpost is very good. Maybe somebody might be willing to turn the result into a "Wochenaufgabe".

@karussell karussell closed this Sep 14, 2015
@karussell karussell reopened this Sep 14, 2015
karussell added a commit that referenced this pull request Sep 14, 2015
Fix parsing of duration tag for hh:mm:ss format.
@karussell karussell merged commit db6f9cf into graphhopper:master Sep 14, 2015
@karussell karussell added the bug label Sep 14, 2015
@karussell karussell added this to the 0.6 milestone Sep 14, 2015
ratrun added a commit to ratrun/biketourplanner that referenced this pull request Sep 14, 2015
This change is the result from a discussion of PR graphhopper#514: Now we are supporting the ISO 8601 format of the duration tag. In case that errors of the tag values are detected, the respective OSMWay ID and the duration tag value gets logged.
ratrun added a commit to ratrun/biketourplanner that referenced this pull request Sep 14, 2015
This change is the result from a discussion of PR graphhopper#514: Now we are supporting the ISO 8601 format of the duration tag. In case that errors of the tag values are detected, the respective OSMWay ID and the duration tag value gets logged.
ratrun added a commit to ratrun/biketourplanner that referenced this pull request Sep 16, 2015
This change is the result from a discussion of PR graphhopper#514: Now we are supporting the ISO 8601 format of the duration tag. In case that errors of the tag values are detected, the respective OSMWay ID and the duration tag value gets logged.
ratrun added a commit to ratrun/biketourplanner that referenced this pull request Sep 20, 2015
This change is the result from a discussion of PR graphhopper#514: Now we are supporting the ISO 8601 format of the duration tag. In case that errors of the tag values are detected, the respective OSMWay ID,the duration tag value and its interpretation in minutes gets logged.
During the discussion in PR#519 the duration tag parsing was moved into reader: Now we perform the duration parsing during reading in order to avoid ISO8601 XML usage within the flag encoders and convert the value into a graphhopper specific "duration:seconds" tag.
The "P2M" test had to be relaxed because on tarvis the openjdk7 obviously returns a slightly different result.
ratrun added a commit to ratrun/biketourplanner that referenced this pull request Sep 20, 2015
This change is the result from a discussion of PR graphhopper#514: Now we are supporting the ISO 8601 format of the duration tag. In case that errors of the tag values are detected, the respective OSMWay ID,the duration tag value and its interpretation in minutes gets logged.
During the discussion in PR#519 the duration tag parsing was moved into reader: Now we perform the duration parsing during reading in order to avoid ISO8601 XML usage within the flag encoders and convert the value into a graphhopper specific "duration:seconds" tag.
The "P2M" test had to be relaxed because on tarvis the openjdk7 obviously returns a slightly different result.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants