-
-
Notifications
You must be signed in to change notification settings - Fork 584
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
Route update #487
Route update #487
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Robinlovelace should we use library()
here? We usually avoided using it inside of a chapter..
Yes but it's a one-off. I can see pros and cons of moving this line To the beginning of the chapter. |
@
@Robinlovelace true. How about using |
Pros: ensures people have access to the library to reproduce everything in the book Cons: it's another package that we'll need to add to Thinking that putting at the top, adding Related question: would you mind if we remove |
Tried it. The package must be loaded unfortunately. |
To be fair, the package doesn't have that many deps: tools::package_dependencies("osrm")
#> $osrm
#> [1] "jsonlite" "RCurl" "utils" "stats" "lwgeom" "isoband"
#> [7] "methods" "gepaf" "sp" "sf" Created on 2020-04-05 by the reprex package (v0.3.0) |
|
@Nowosad you can test it, e.g. with the reproducible example below: osrm::osrmRoute(c(-1.5, 53.8), c(-1.51, 53.81))
#> The OSRM server returned an error:
#> Error in function (type, msg, asError = TRUE) : Could not resolve host: route
#> NULL
library(osrm)
#> Data: (c) OpenStreetMap contributors, ODbL 1.0 - http://www.openstreetmap.org/copyright
#> Routing: OSRM - http://project-osrm.org/
osrm::osrmRoute(c(-1.5, 53.8), c(-1.51, 53.81))
#> lon lat
#> 1 -1.49920 53.79990
#> 2 -1.49765 53.80018
#> 3 -1.49773 53.80139
#> 4 -1.49827 53.80134
#> 5 -1.49888 53.80158
#> 6 -1.50766 53.80777
#> 7 -1.51009 53.80981
#> 8 -1.50988 53.80990 Created on 2020-04-05 by the reprex package (v0.3.0) I'm not sure why but there are many packages (including sf) that must be loaded for their full functionality to work. Heads-up @rCarto do you have any insights into this? I don't think it's a bug but just wondering. |
To return to the question of how to fix #486, what do you think about this as a way forward given that
|
I am fine with that. |
Hi, The problem here is that the URL of the default OSRM server (the demo server) is set on package attach (see https://github.com/rCarto/osrm/blame/master/R/zzz.R). options(osrm.server = "http://router.project-osrm.org/", osrm.profile = "driving") I have to look into it further and find a better solution to set the default server (if you have a suggestion...). Side notes: This one is more worrying: Project-OSRM/osrm-backend#5463 |
Hi @rCarto, many thanks for the timely and clear response, makes sense to me, we will go with
for now as that's simpler and simplicity is good when teaching people, many of whom may be new to R. Regarding development of OSRM, I was aware of this development, 'Mapbox ditched osrm' in the words of @mvl22 who is involved in the development of a cycling-specific routing service, CycleStreets.net which may be of interest - see https://github.com/Robinlovelace/cyclestreets/ for a mini R package that may be of interest, you just need a free API key and their service is highly recommended. I think OpenRouteService looks promising, plus OTP, which can be accessed via https://github.com/ropensci/opentripplanner ... Lots going on, OSRM is solid and fast though and not going anywhere as far as I can tell so is good as a basis for teaching routing IMO. Any other comments very welcome, looking forward to it all (and still on for teaching in France hopefully). |
Project-OSRM/osrm-backend#5209 is the main ticket you want to see. "18 months on, I think we can probably say that OSRM is to all intents and purposes abandoned." |
Update: added |
Ready to merge I think @Nowosad. Has implications for |
No description provided.