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

Change speed unit from m/s*100 to mm/s in Edges #295

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Conversation

buma
Copy link
Contributor

@buma buma commented Jul 3, 2017

This also changes calculations for time when traversing from m / m/s100 -> mm/ mm/s. Which means 2 divisions less (mm-> m and m/s100 -> m/s). CalculateSpeed function returns speed in mm/s now and ProfileRequest#getSpeedForMode also in mm/s since speed was always multiplied by 1000 where it was used. Everything should work the same as before since I checked all the usages but I can't test analyst.

Currently speeds for walk,bike and car are saved as m/s in ProfileRequest and are multiplied by 1000 on each getSpeedForMode call. Should this be changed? Since this is called on each traverse with walk/bike mode.

buma added 3 commits June 30, 2017 11:29
Time in traverse is calculated with mm length and mm/s
GetSpeedForMode is now in mm/s not m/s
@mattwigway
Copy link
Contributor

I don't think we want to change the profilerequest values to not be in m/s since that API is used by the outside world and most people are not familiar with mm/s. However we may want to think about how to eliminate the cast on each traverse, that may well be why street searches are slow.

@buma
Copy link
Contributor Author

buma commented Jul 31, 2017

Fixed conflicts because file was deleted. And tested again with my saved requests.

@abyrd
Copy link
Member

abyrd commented Oct 13, 2020

Revisiting this in October 2020, after combining r5 with analysis-backend. Because this is a pure refactor and should not change results, but does touch core routing code, it is probably best to do this after creating a round of regression tests.

@abyrd abyrd closed this Apr 9, 2021
@abyrd abyrd deleted the branch dev April 9, 2021 04:50
@abyrd abyrd reopened this Apr 9, 2021
@abyrd abyrd changed the base branch from remove-cruft to dev April 9, 2021 05:24
@abyrd
Copy link
Member

abyrd commented Apr 9, 2021

Accidentally closed when cleaning up branches. I would like to revisit these old pull requests and determine whether they're still relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants