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

Adapt interface in order to improve package interoperability #42

Closed
Robinlovelace opened this issue Mar 20, 2019 · 9 comments · Fixed by #49
Closed

Adapt interface in order to improve package interoperability #42

Robinlovelace opened this issue Mar 20, 2019 · 9 comments · Fixed by #49
Assignees
Labels
💡 enhancement New feature or request
Milestone

Comments

@Robinlovelace
Copy link

First to say: many thanks for a great package!

Wondering if you have plans to submit it to CRAN. Context: thinking of writing a wrapper, e.g. route_ors(), in stplanr. We've written some code here that uses it, rather inefficiently at the moment!: https://github.com/pedalea/pctSantiago

@aoles aoles added the question Further information is requested label Mar 21, 2019
@aoles aoles self-assigned this Mar 21, 2019
@aoles
Copy link
Member

aoles commented Mar 21, 2019

Dear Robin,

many thanks for reaching out. Awesome job with stplanr!

Sure, the ultimate goal is to publish the package on CRAN. Before that there are still some issues which I believe need to be addressed. In particular, as I consider the package to be in development stage there might be some slight changes to the package API like adjustments to function names or function arguments such as #39 and #33.

Which leads me to the question: what is the primary need for route_ors wrapper? If the problem is the structuring of input/output formats I'm open to discussion of possible improvements on our side for better interoperability of packages.

Cheers,
Andrzej

@Robinlovelace
Copy link
Author

Hey @aoles great to hear. Yes it's more a question of output. I suspect many R users will find it useful for the function to return an sf object. In terms of inputs... That's a different matter - there' a need for consistency and @mem48 have discussed ways of doing this, with matrices of origin-destination pairs being a useful input format I think. If it returns sf linestrings, stplanr could just import it and use it as another option in route(FUN = ...) calls would be the plan. See ropensci/stplanr#283 for more on my plans going forward.

@aoles aoles pinned this issue Mar 21, 2019
@aoles
Copy link
Member

aoles commented Mar 21, 2019

Thanks a lot for your feedback!

The input to ors_directions is in a way predefined by openrouteservice API itself as the original idea of the package was to provide easy access to our services from R. But we could of course extend the R interface by adding from and to arguments as an alternative way of specifying origin-destination pairs. As a side note, we could maybe do the same for ors_matrix: provide an alternative way of specifying the pairing by from and to arguments rather than locations and destinations.

Regarding the output format I will explore ways of improving support for sf objects. Of course, the easiest option would be to extend the list of possible output formats of ors_directions to return a linestring. However, considering that openrouteservice response is much more complex than just the route geometry, I'm not entirely sure that this is the cleanest solution. I need to give it a thought, so stay tuned.

looking forward to the exciting collaboration between our packages ;)

Cheers,
Andrzej

@aoles aoles added the 💡 enhancement New feature or request label Mar 21, 2019
@aoles aoles changed the title Plans for submission to CRAN Adapt interface in order to improve package interoperability Mar 21, 2019
@Robinlovelace
Copy link
Author

Robinlovelace commented Mar 21, 2019

Great. I can have a first bash at implementing output = ors_sf (which should contain the attribute and geometry info).

@aoles aoles removed the question Further information is requested label Mar 21, 2019
Robinlovelace added a commit to Robinlovelace/openrouteservice-r that referenced this issue Mar 26, 2019
@aoles
Copy link
Member

aoles commented Apr 10, 2019

Dear Robin,

many thanks for taking the time to look into this and for submitting your pull request. After careful consideration I have decided to adapt some of the package interface in order to natively support sf output. Feel free to give it a try on the output_sf branch.

Cheers,
Andrzej

@Robinlovelace
Copy link
Author

Great work. Not tried but, after looking at the code, it seems like a sensible solution. One question: is it possible to send a list of requests to be routed? For example, by sending a matrix of origin-destination pairs (I know you can send a list of waypoints but what about many trips that start and end in different places?) like this, to calculate the routes for 2 lines:

stplanr::line2df(stplanr::flowlines_sf[2:3, ])
#> # A tibble: 2 x 5
#>      L1    fx    fy    tx    ty
#>   <dbl> <dbl> <dbl> <dbl> <dbl>
#> 1     1 -1.52  53.8 -1.54  53.8
#> 2     2 -1.52  53.8 -1.55  53.8

Created on 2019-04-11 by the reprex package (v0.2.1)

In stplanr you can currently do this as follows:

library(stplanr)
r = line2route(flowlines_sf[2:3, ], route_fun = route_cyclestreet)
plot(r)
#> Warning: plotting the first 10 out of 17 attributes; use max.plot = 17 to
#> plot all

Created on 2019-04-11 by the reprex package (v0.2.1)

@aoles aoles modified the milestones: version 0.3.0, version 0.2.0 Apr 12, 2019
@aoles
Copy link
Member

aoles commented Apr 17, 2019

Dear Robin,

thanks for your feedback! The openrouteservice routing API which ors_directions() interfaces is designed to take just a single route at a time. Considering the fact that the package aims to act as a lightweight client to the API I believe that covering more complicated input scenarios is not our highest priority at the moment, I'm afraid. Rather, we would like to focus on addressing any remaining issues before releasing the package on CRAN.

In particular, querying for a list of routes is relatively simple to implement by the end user. In contrary, a vectorized package function might easily become complicated and quite obscure considering all the possible route configuration options. Because of that I don't see an urgent need for addressing this issue by designing a generic solution in the package.

For now I would suggest that you consider providing a route_ors() wrapper in stplanr (probably similar to route_graphhopper()) and handle multi-route request on your side.

Cheers,
Andrzej

@Robinlovelace
Copy link
Author

That sounds reasonable - batch routing is a specific task that can make life more complicated so happy to approach this on the stplanr end.

Many thanks for the reply, and excellent work on support for sf outputs. I think this issue can be closed when the https://github.com/GIScience/openrouteservice-r/tree/output_sf is merged into master. Any further feedback you'd like from me: just say.

@Robinlovelace
Copy link
Author

Great work @aoles many thanks.

@aoles aoles unpinned this issue May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants