Changed calculation of slope costs (#2579) #2580

gmellemstrand · 2018-06-21T12:42:15Z

This is an implementation we have had in production at Entur for about 6 months now. It is quite safe to use, as it caps the effective length factor for slopes between 1 and 3. We should probably come up with a more complete formula that accounts for steeper slopes and possible speed increases for downhill slopes. This will require more analysis, however, as some small street edges can be really steep and that does not play well with an exponential function.

…e multiplier.

t2gran · 2018-10-19T08:24:14Z

src/main/java/org/opentripplanner/routing/util/ElevationUtils.java

        }
        /*
         * Here we divide by the *flat length* as the slope/work cost factors are multipliers of the
         * length of the street edge which is the flat one.
         */
        return new SlopeCosts(slopeSpeedEffectiveLength / flatLength, slopeWorkCost / flatLength,
-                slopeSafetyCost, maxSlope, lengthMultiplier, flattened);
+                slopeSafetyCost, maxSlope, lengthMultiplier, flattened, slopeWalkEffectiveLength * walkParA / flatLength);


This is wrong. The slopeWalkEffectiveLength should be used as is. There is no speed component in this any more. The * walkParA / flatLeng can be removed.

When this is ready (@t2gran approves it) I'll provide another review to allow merging.

Strictly I have participated in the development of this, so maybe someone else should review this in addition too @abyrd. @optionsome has done some testing on this in Finland, so maybe we can send an invitation to @tuukka and he can suggest some people that can be added to the list of reviewers. I believe adding people must be approved by the PLC.

…ctor `alkParA` from the calculation. - Cleanup names and comments. - Fix tests.

abyrd · 2018-11-29T10:15:07Z

@optionsome I see that you were commenting on issue #2579 and have already done some testing of this new walk slope system. Can you provide a review of this pull request?

optionsome

We observed issues with edge splitting in our fork. We fixed those and thought that they were not related to this pull request (which we have already merged into our fork). However, now that I started to review this again, I tested upstream's master with and without these changes and seems like these changes are causing the issue. The issue is that the walk time for first/last edge is not correct. The first picture is with these changes.

The second picture is without these changes.

We have already created a fix for this issue and it seems to work. It can be found here. The last commit there is not related to this issue.

…ance 0 (zero) by setting effectiveWalk distance as coefficient instead of mm. (cherry picked from commit 1818955) (cherry picked from commit d8abd1d) (cherry picked from commit 45df71f)

t2gran · 2018-12-05T18:00:31Z

Thank you @optionsome for taking the time to test this, and providing a fix. I was a puzzled by this, but the error is that the elevation data does not propagate into the TemporaryEdges when split. There is at least 3 ways to solve this.

The best solution would be to recalculate the "slope factor" after the split, so the new edges would be correct. See comments in SimpleStreetSpliter line 366.
We can also use the same factor as the splitt edge. The provided fix do that. This will however give a wrong result if there is a steep slope towards the end of the original edge, and the temporary edge happens to travers just this part.
We can ignore the slop factor and use the flat length.

I have reproduced the problem and tested that it works.

pailakka · 2018-12-10T12:01:44Z

option would be best, but isn't elevation data only available on graph build stage?

The solution in the fix (option 2) has indeed this drawback but I don't think that it is that significant in majority of cases. in most cases edges are rather short and I don't think that it is that problematic, long edges in steep terrain would of course produce larger error.

Option 3 is should be viable, although it has same shortcomings as option 2.

gmellemstrand · 2018-12-11T07:56:54Z

@pailakka We do actually have the elevation data when routing. It is contained in StreetWithElevationEdge.packedElevationprofile. I think it could be a good idea to go with option 1, as I don't think it will cost much in terms of performance.

pailakka · 2018-12-11T08:18:14Z

I stand corrected, elevation data is indeed available when routing. Calculation needs to be done only twice with every RoutingRequest so it shouldn't be too bad performance wise.

…it. This apply to biking and walking.

t2gran · 2019-01-11T18:33:46Z

I have now fixed the elevation calculation for temporary edges. The fix calculate a new elevation profile for the partial section of the original edge. (Solution 1 above). This fix apply to both biking and walking.
@optionsome / @pailakka would you mind testing it and then approve this PR if it works.
@abyrd Can you review the code changes of the last commit?

optionsome

Didn't find problems while testing these changes but couple of comments should be slightly updated.

optionsome · 2019-01-16T12:05:14Z

src/main/java/org/opentripplanner/graph_builder/linking/SimpleStreetSplitter.java

-        // make the edges
-        // TODO this is using the StreetEdge implementation of split, which will discard elevation information
-        // on edges that have it
+        // make the edges on edges that have it


This comment already existed here earlier but I find it slightly hard to understand. Maybe it could be worded a bit better.

optionsome · 2019-01-16T12:13:56Z

src/main/java/org/opentripplanner/routing/edgetype/PartialStreetEdge.java

@@ -31,8 +32,13 @@ public PartialStreetEdge(StreetEdge parentEdge, StreetVertex v1, StreetVertex v2
        super(v1, v2, geometry, name, length, parentEdge.getPermission(), false);
        setCarSpeed(parentEdge.getCarSpeed());
        this.parentEdge = parentEdge;
+
+        // Initialize length_mm from Geometry
+        if(getLength_mm() == 0) {


I think it could be a good idea to explain here in a comment why the length is sometimes 0 and why it needs to be calculated here.

Agree, also this is the only place length_mm is used outside its StreetEdge, I will move the logic to StreetEdge to iImprove encapsulation.

I refactored this so we that there is one constructor in the TemporaryPartialStreetEdge with length and one without, and JavaDoc that explain the difference.

…n edge. - Push assigning 'wheelchairAccessible' into the 'TemporarySplitterVertex' constructor. - Create constructor for temporary street edge without a pre-calculated length, calculate length from the geometry in this case. - Update some comments

…ialStreetEdge, witch were the only place it was used except for tests. - Simplified the constructors a bit.

t2gran · 2019-01-17T14:52:45Z

@optionsome I decided to refactor instead of explaining the implementation (fixing the documentation requested changes). Hopefully this become more clear. I have added som comments as well.

@abyrd I removed the PartialStreetEdge - it was just simpler to refactor it into the TemporaryPartialStreetEdge(TPSE), then to describe the problem in a new issue. The TemporaryPartialStreetEdge had very specific constructors (5 of them); witch limited the type of edges we could link with a TemporaryEdge; I think it is the callers responsibility to enforce such thing, not the TPSE. If the types were used inside the TPSE; then it would be something else. If the intention was to ensure that at least one of the start/end Vertices is a Temporary vertex; then do it explicit in like an assertAtLEastOneVertexIsTemporary(v1, v2); - not by adding a new constructor for each possible combination of types.

abyrd

This looks good to me, thanks @t2gran for the fixes to edge splitting. Only one request for changes, which is a spelling issue: I believe ToblersHickingFunction.java should be "hiking" rather than "hicking". This spelling appears in about 13 places throughout the PR. With that simple spelling correction, I will approve it. So then just waiting for approval from @optionsome.

@t2gran about the many constructors with specific type parameters: this is from a time when we were trying to use language features to enforce graph structure. It was useful to know with certainty that types of edges were always assembled in the same way. It is indeed more concise and readable to do this with an assertion - I think the six constructors here are the result of someone seeing a pattern and taking it a step too far. In the ideal case, we'd enforce lots of things with language features at compile time and avoid instanceof which is best avoided... but the reality is that OTP is still full of instanceof so there's no reason not to use it here.

t2gran · 2019-01-18T09:40:00Z

@abyrd I think restricting the types is a good thing, but only down to what is important/used - in this case is temporary or not - allowing new types to be created. The instanceof is really a result of using a marker interface. I am more and more convinced that the solution to this problem is to keep temporary objects in the request context, and NOT change the permanent stuff - the problem is of cause to find a good and performing way of to connect the permanent stuff to the temporary. Thank you again for explaining the history and reason why things are as they are, it is good to know and helps on the understanding of the code.

optionsome · 2019-01-21T12:13:13Z

@abyrd this branch could be merged into dev-1.x soon after travis is done building. However, I just read from "Branches and Branch Protection" instructions that merges should be done with fast-forward. I think github merge uses --no-ff by default but has that been changed for this repository?

abyrd · 2019-01-22T06:58:16Z

@optionsome I think you are referring to a section of the OTP documentation at https://opentripplanner.readthedocs.io/en/latest/Developers-Guide/#contributing-to-the-project - there it does say "All other changes to master should result from fast-forward merges of a Github pull request from the dev-1.x branch". I was using the expression "fast forward" to mean that a merge is not necessary, that master does not contain any commits that are not in dev-1.x, so merging results in exactly the same code as moving the HEAD of master to match the HEAD of dev-1.x.

As you point out there is a distinction between a branch that can be fast-forwarded, and actually doing a fast-forward instead of a merge. We did not remove the --no-ff option on this repository, I just didn't consider the fact that Github always creates merge commits. It looks like you can choose a merge method (https://help.github.com/articles/about-merge-methods-on-github/) but for our purposes the default method with a merge commit is fine.

I just need to reword the documentation to clarify the difference between "can be fast-forwarded" and "true fast-forward merge".

Changed calculation of slope costs

65bce5e

t2gran mentioned this pull request Jul 18, 2018

Slope walking speed calculation is not accurate #2579

Closed

Implement the Tobler Hicking Function for calculation of walk distanc…

2f96c21

…e multiplier.

gmellemstrand requested a review from a team as a code owner July 27, 2018 15:47

t2gran reviewed Oct 19, 2018

View reviewed changes

- Fix error in ElevationUtils.java and remove the speed adjustment fa…

ba4a09e

…ctor `alkParA` from the calculation. - Cleanup names and comments. - Fix tests.

abyrd requested a review from optionsome November 29, 2018 10:15

optionsome requested changes Dec 5, 2018

View reviewed changes

Fix bug causing temporary split edges to be of effective walking dist…

a0dfec3

…ance 0 (zero) by setting effectiveWalk distance as coefficient instead of mm. (cherry picked from commit 1818955) (cherry picked from commit d8abd1d) (cherry picked from commit 45df71f)

Merge branch 'master' into slope_walk_speed

fd34ad2

t2gran mentioned this pull request Jan 10, 2019

No elevation data in TemporaryEdges #2705

Closed

Recalculate the the elevation profile for temporary edges after a spl…

6dea1cf

…it. This apply to biking and walking.

optionsome reviewed Jan 16, 2019

View reviewed changes

t2gran changed the base branch from master to dev-1.x January 17, 2019 11:40

t2gran added 2 commits January 17, 2019 14:57

Merge remote-tracking branch 'otp/dev-1.x' into slope_walk_speed

4c0e50c

- Merged super class PartialStreetEdge into child class TemporaryPart…

df68fe1

…ialStreetEdge, witch were the only place it was used except for tests. - Simplified the constructors a bit.

abyrd requested changes Jan 18, 2019

View reviewed changes

Spelling 'hicking' corrected into 'hiking'.

a00de6b

abyrd approved these changes Jan 18, 2019

View reviewed changes

optionsome approved these changes Jan 21, 2019

View reviewed changes

Merge branch 'dev-1.x' into slope_walk_speed

e1736a2

abyrd merged commit c8f6014 into opentripplanner:dev-1.x Jan 22, 2019

evansiroky mentioned this pull request Jul 11, 2019

Vehicle rental edge split refactor ibi-group/OpenTripPlanner#5

Merged

t2gran deleted the slope_walk_speed branch September 24, 2019 14:16

mvpsaraiva mentioned this pull request Oct 28, 2020

Comparação entre roteadores #2: efeito da topografia nas viagens a pé e de bicicleta ipeaGIT/acesso_oport#62

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed calculation of slope costs (#2579) #2580

Changed calculation of slope costs (#2579) #2580

gmellemstrand commented Jun 21, 2018

t2gran Oct 19, 2018

abyrd Oct 19, 2018

t2gran Oct 19, 2018

abyrd commented Nov 29, 2018

optionsome left a comment •

edited

Loading

t2gran commented Dec 5, 2018

pailakka commented Dec 10, 2018

gmellemstrand commented Dec 11, 2018

pailakka commented Dec 11, 2018

t2gran commented Jan 11, 2019 •

edited

Loading

optionsome left a comment

optionsome Jan 16, 2019

optionsome Jan 16, 2019

t2gran Jan 16, 2019 •

edited

Loading

t2gran Jan 17, 2019

t2gran commented Jan 17, 2019

abyrd left a comment

t2gran commented Jan 18, 2019

optionsome commented Jan 21, 2019

abyrd commented Jan 22, 2019

Changed calculation of slope costs (#2579) #2580

Changed calculation of slope costs (#2579) #2580

Conversation

gmellemstrand commented Jun 21, 2018

t2gran Oct 19, 2018

Choose a reason for hiding this comment

abyrd Oct 19, 2018

Choose a reason for hiding this comment

t2gran Oct 19, 2018

Choose a reason for hiding this comment

abyrd commented Nov 29, 2018

optionsome left a comment • edited Loading

Choose a reason for hiding this comment

t2gran commented Dec 5, 2018

pailakka commented Dec 10, 2018

gmellemstrand commented Dec 11, 2018

pailakka commented Dec 11, 2018

t2gran commented Jan 11, 2019 • edited Loading

optionsome left a comment

Choose a reason for hiding this comment

optionsome Jan 16, 2019

Choose a reason for hiding this comment

optionsome Jan 16, 2019

Choose a reason for hiding this comment

t2gran Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

t2gran Jan 17, 2019

Choose a reason for hiding this comment

t2gran commented Jan 17, 2019

abyrd left a comment

Choose a reason for hiding this comment

t2gran commented Jan 18, 2019

optionsome commented Jan 21, 2019

abyrd commented Jan 22, 2019

optionsome left a comment •

edited

Loading

t2gran commented Jan 11, 2019 •

edited

Loading

t2gran Jan 16, 2019 •

edited

Loading