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

Route update #487

Merged
merged 3 commits into from
Apr 8, 2020
Merged

Route update #487

merged 3 commits into from
Apr 8, 2020

Conversation

Robinlovelace
Copy link
Collaborator

No description provided.

Copy link
Member

@Nowosad Nowosad left a 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..

@Robinlovelace
Copy link
Collaborator Author

Yes but it's a one-off. I can see pros and cons of moving this line

https://github.com/Robinlovelace/geocompr/blob/55225735e71a32da658b50659a867ff0f18c6e56/12-transport.Rmd#L358

To the beginning of the chapter.

@Nowosad
Copy link
Member

Nowosad commented Apr 5, 2020

@

Yes but it's a one-off. I can see pros and cons of moving this line

https://github.com/Robinlovelace/geocompr/blob/55225735e71a32da658b50659a867ff0f18c6e56/12-transport.Rmd#L358

To the beginning of the chapter.

@Robinlovelace true. How about using osrm::osrmRoute?

@Robinlovelace
Copy link
Collaborator Author

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 geocompkg that will slow build times

Thinking that putting at the top, adding osrm to geocompkg and running the line of code may be be best, we can always return to eval=F if the service goes down. Sound like a plan?

Related question: would you mind if we remove RSAGA from the DESCRIPTION in geocompkg? It's a pain to install and is rarely used.

@Robinlovelace
Copy link
Collaborator Author

@Robinlovelace true. How about using osrm::osrmRoute?

Tried it. The package must be loaded unfortunately.

@Robinlovelace
Copy link
Collaborator Author

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
Copy link
Member

Nowosad commented Apr 5, 2020

@Robinlovelace:

  1. I will suggest keeping RSAGA as we are using it in the 9 chapter - https://geocompr.robinlovelace.net/gis.html#rsaga

@Robinlovelace true. How about using osrm::osrmRoute?

Tried it. The package must be loaded unfortunately.

  1. Why it must be loaded? What kind of side-effect happens here? Maybe it is worth investigating...

@Robinlovelace
Copy link
Collaborator Author

@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.

@Robinlovelace
Copy link
Collaborator Author

To return to the question of how to fix #486, what do you think about this as a way forward given that osrm needs to be loaded @Nowosad ?

Thinking that putting at the top, adding osrm to geocompkg and running the line of code may be be best, we can always return to eval=F if the service goes down. Sound like a plan?

@Nowosad
Copy link
Member

Nowosad commented Apr 5, 2020

To return to the question of how to fix #486, what do you think about this as a way forward given that osrm needs to be loaded @Nowosad ?

Thinking that putting at the top, adding osrm to geocompkg and running the line of code may be be best, we can always return to eval=F if the service goes down. Sound like a plan?

I am fine with that.

@rCarto
Copy link

rCarto commented Apr 5, 2020

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).
So you may add this line somewhere :

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:
The demo server is running for the moment but this is likely to change soon: Project-OSRM/osrm-backend#5655
This is not a problem for osrm R package, as it works fine with any other instance of OSRM.
But it will certainly make occasional use more difficult.

This one is more worrying: Project-OSRM/osrm-backend#5463

@Robinlovelace
Copy link
Collaborator Author

Hi @rCarto, many thanks for the timely and clear response, makes sense to me, we will go with

library(osrm)

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).

@mvl22
Copy link
Contributor

mvl22 commented Apr 5, 2020

Regarding development of OSRM

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."

@Robinlovelace
Copy link
Collaborator Author

Robinlovelace commented Apr 5, 2020

Update: added osrm to geocompkg and it adds only gepaf and osrm to our dependencies, as can be seen here: https://travis-ci.org/github/Robinlovelace/geocompr/jobs/671384897#L1734

@Robinlovelace
Copy link
Collaborator Author

Ready to merge I think @Nowosad. Has implications for spDataLarge but that's a separate issue that we can raise if/when this solution goes on master and out to the masses reading our 📖

@Nowosad Nowosad merged commit 3376fc4 into master Apr 8, 2020
@Robinlovelace Robinlovelace deleted the route-update branch April 8, 2020 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants