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

Enable uber_estimate_price to use addresses #1

Closed
jonocarroll opened this issue Sep 6, 2016 · 5 comments
Closed

Enable uber_estimate_price to use addresses #1

jonocarroll opened this issue Sep 6, 2016 · 5 comments

Comments

@jonocarroll
Copy link
Contributor

I was going to make this a PR to add ggmap::geocode() processing to uber_estimate_price, but on inspecting that function, and thus ubeR:::parseParameters, I noticed that you've already implemented half of what's needed for this (you already process the parameters, they just aren't arguments).

This issue is therefore to request the completion of that addition, to optionally allow addresses as arguments to uber_estimate_price.

I'd suggest you add a little more processing to the geocode parsing to catch when a result can't be found. For example,

g1 <- geocode("Mouille Point")

works fine, but

g2 <- geocode("The Old Biscuit Mill")

produces geocode failed with status ZERO_RESULTS. By the looks of parseParameters this will flow through as NA. You may want it to fail early. I need to set that one to

g2 <- geocode("The Old Biscuit Mill, Woodstock, Cape Town")

then it works fine.

If you really want some flashy new feature, I have some code to produce the Google Map route between the two requested points (via the googleway package) though it requires an API key

capetown

Everything else checks out nicely. Great work!

@datawookie
Copy link
Owner

Thanks! This is a great suggestion. I'll get onto the implementation later in the week.

@datawookie
Copy link
Owner

Hi Jono,

Okay, I have:

  1. Changed geocode() to throw an error if it fails to find a location.
  2. Added start and end addresses as options to the estimate functions.

Would you mind testing from the GitHub version of the package?

Your googleway package looks awesome! Do you have a suggestion for how this might be integrated into ubeR?

Best regards,
Andrew.

@jonocarroll
Copy link
Contributor Author

jonocarroll commented Sep 8, 2016

I'll take it for a test drive tonight and let you know if I find anything not working.

If you like, I can create a pull request with some code to add a mapping function, potentially callable from the price estimate (or independently).

My example image was created using that code and I can wrap it up into a function compatible with your package

The googleway package isn't mine, I just provided some comments and used in some of my own work. Credit goes to @SymbolixAU for that one. It's on CRAN now, so you can make it a formal dependency (assuming you're going to release on CRAN too).

I'll need to see if there's a way around requiring a Google Maps API key... I suppose I could create a public one and have a request not to abuse it. All good to go.

@datawookie
Copy link
Owner

That'd be great! Thanks, Jonathan, I look forward to seeing that.

We are planning on a submission to CRAN next week and it'd be great to
include that functionality too.

On 08/09/2016 04:18, Jonathan Carroll wrote:

I'll take it for a test drive tonight and let you know if I find
anything not working.

If you like, I can create a pull request with some code to add a
mapping function, potentially callable from the price estimate (or
independently).

My example image was created using that code and I can wrap it up into
a function compatible with your package
https://cloud.githubusercontent.com/assets/9496865/18274831/9990ae42-7483-11e6-8e13-eb131c4c70e4.png

The |googleway| https://github.com/SymbolixAU/googleway package
isn't mine, I just provided some comments and used in some of my own
work. Credit goes to @SymbolixAU https://github.com/SymbolixAU for
that one. It's on CRAN now, so you can make it a formal dependency
(assuming you're going to release on CRAN too).

I'll need to see if there's a way around requiring a Google Maps API
key... I suppose I could create a public one and have a request not to
abuse it.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF2aiSx6PLmsOcNsAQRHdUooSy1k1NqBks5qn3BxgaJpZM4J1024.

Andrew B. Collier, PhD http://www.exegetic.biz
Exegetic Analytics +27 83 3813655
http://za.linkedin.com/in/collierab/ skype: abcollier

@jonocarroll
Copy link
Contributor Author

I made a slight change to parseParameters because it was seeing the names that it was testing against, even though the value was NULL. Presumably, you mean to allow either coordinates or addresses; at the moment you take both and overwrite with the coordinates if they're there.

I edited the example coordinates (somewhere in SF) but there was only the start (thus the example fails) so I added a random end location. Update to something meaningful if you wish.

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

No branches or pull requests

2 participants