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

Better escalator duration control: specific duration from OSM duration tag, default speed from build-config.json #6268

Merged
merged 17 commits into from
Dec 9, 2024

Conversation

tkalvas
Copy link
Contributor

@tkalvas tkalvas commented Nov 22, 2024

Summary

Escalator durations tend too high. This PR makes it possible to use duration=hh:mm:ss tag in OSM data to set the duration accurately. It also makes it possible to set a configuration-wide speed in build-config.json at routingDefaults.escalatorSpeed.

Issue

None.

Unit tests

New case in EscalatorEdgeTest: if a duration is set for the edge, it takes precedence over a calculated duration.

Documentation

doc/user/RouteRequest.md has been autogenerated

Changelog

Definitely will be included in changelog.

Bumping the serialization version id

No

@tkalvas tkalvas requested a review from a team as a code owner November 22, 2024 10:13
@tkalvas tkalvas marked this pull request as draft November 22, 2024 10:14
@tkalvas
Copy link
Contributor Author

tkalvas commented Nov 22, 2024

Will be a draft until I have completed my full-program manual testing.

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 73.17073% with 33 lines in your changes missing coverage. Please review.

Project coverage is 69.79%. Comparing base (62c3d7b) to head (ffffb13).
Report is 253 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...r/graph_builder/module/osm/EscalatorProcessor.java 27.77% 12 Missing and 1 partial ⚠️
...g/api/request/preference/EscalatorPreferences.java 74.35% 8 Missing and 2 partials ⚠️
...ava/org/opentripplanner/osm/model/OsmWithTags.java 82.14% 1 Missing and 4 partials ⚠️
...nner/inspector/vector/edge/EdgePropertyMapper.java 0.00% 3 Missing ⚠️
...outing/api/request/preference/WalkPreferences.java 87.50% 1 Missing ⚠️
...entripplanner/street/model/edge/EscalatorEdge.java 85.71% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6268      +/-   ##
=============================================
+ Coverage      69.71%   69.79%   +0.07%     
- Complexity     17698    17816     +118     
=============================================
  Files           2008     2020      +12     
  Lines          75830    76221     +391     
  Branches        7764     7799      +35     
=============================================
+ Hits           52868    53201     +333     
- Misses         20248    20308      +60     
+ Partials        2714     2712       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tkalvas tkalvas marked this pull request as ready for review November 25, 2024 12:42
@optionsome optionsome added bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Improvement labels Nov 26, 2024
@optionsome optionsome added this to the 2.7 (next release) milestone Nov 26, 2024
@@ -459,6 +460,16 @@ private static void mapStreetPreferences(NodeAdapter c, StreetPreferences.Builde
.asInt(dftElevator.hopTime())
);
})
.withEscalator(escalator -> {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

This is a good PR but I've picked up on a few things.

* @return Duration
* @throws DateTimeParseException on bad input
*/
public static Duration parseClockDuration(String duration) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we need this or could we just use Duration.between(LocalTime.MIN, LocalTime.parse(duration))? The parse function probably accepts more formats that what is in theory allowed by OSM tagging.

Copy link
Member

Choose a reason for hiding this comment

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

There are also factories in the data API to create custom parsers - we do not need this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Joel also wanted to use Duration.between(LocalTime.MIN, LocalTime.parse(duration, DateTimeFormatter.ISO_LOCAL_TIME)), which I guess will probably work ok for durations smaller than 24 hours. But the format specified in https://wiki.openstreetmap.org/wiki/Key:duration does not limit the duration to under 24 hours, which is of course irrelevant for the escalator use case, but if we ever want to recycle the duration tag parsing code for other durations, it might become relevant. Let's go through this in today's meeting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But now I noticed that my implementation is against the spec anyway. It specifies that the duration is either mm, hh:mm or hh:mm:ss. Not hh, hh:mm or hh:mm:ss as I implemented it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway, the fact that the supported formats are mm, hh:mm, and hh:mm:ss means that there has to be a place for the switch off the number of colons somewhere, even if the branches use some ready-made parser. I'll bring this up in the meeting as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DateTimeFormatterBuilder will not work for this. It has the capability for expressing optional parts, so it could express hh(:mm(:ss)?)? but it cannot express (hh:)?mm(:ss)? where the existence of (:ss) implies the existence of (hh:). Even if it did, it would not be able to handle the cases where hours are greater than 23 or (if there is no hours part at all) minutes are greater than 59, which are both allowed by the spec and exist in OSM data. Durations are not LocalTimes after all, in parsing a LocalTime it makes sense and is correct that hours cannot be more than 23 or minutes more than 59, but in durations if you have capped the largest unit, it is reasonable for the amount of the largest unit to be as large as it needs to be.

I did rewrite the parsing to be quite explicit in what it accepts, so its correctness should be obvious.

@tkalvas
Copy link
Contributor Author

tkalvas commented Nov 28, 2024

While going through the comments, I noticed our ElevatorEdge implementation uses the OSM tag duration as an integer number of seconds, which is just wrong, compared to the duration tag spec. We couldn't even find an elevator tagged like this anywhere.

issueStore.add(
Issue.issue(
"InvalidDuration",
"Duration for osm node %d is not a valid duration: '%s'; it's replaced with 'Optional.empty()' (unknown).",
Copy link
Member

Choose a reason for hiding this comment

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

I think you could use a link to OSM instead of just referencing the id (nodes have url() method). Also, the "it's replaced with 'Optional.empty()'" is too technical for the issue report. Instead you could just say something like "the value is ignored".

issueStore.add(
Issue.issue(
"InvalidDuration",
"Duration for osm node {} makes implied speed {} be outside acceptable range.",
Copy link
Member

Choose a reason for hiding this comment

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

Use URL here as well.

optionsome
optionsome previously approved these changes Dec 9, 2024
Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

I approve this. The typo I can fix after the merge.

@optionsome optionsome dismissed stale reviews from leonardehrenfried and themself via ffffb13 December 9, 2024 12:04
@optionsome optionsome merged commit 5a4fdfe into opentripplanner:dev-2.x Dec 9, 2024
4 checks passed
t2gran pushed a commit that referenced this pull request Dec 9, 2024
@optionsome optionsome deleted the escalator-duration branch December 9, 2024 12:05
t2gran pushed a commit that referenced this pull request Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants