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

pmap (almost) all the things #67

Merged
merged 93 commits into from
May 8, 2019
Merged

pmap (almost) all the things #67

merged 93 commits into from
May 8, 2019

Conversation

dpprdan
Copy link
Member

@dpprdan dpprdan commented May 3, 2019

The main point of the feat_pmap branch is to vectorise (almost) all arguments of oc_forward()/oc_reverse(). This made some code clean_up necessary. The exported gecoding functions basically just wrap oc_check_query and oc_process() which do the main work.

The main changes are:

  1. I implemented NSE in base R rather than tidyeval in oc_forward_df()/oc_reverse_df(). This is probably the part I am most uncertain about, so let me know if you have a better idea on how to implement this.
  2. I added a oc_bbox() helper function, to get the countrycode arguments into shape.
  3. I added a oc_config() function, which only changes the rate limit at the moment (useful if you have one of the paid plans), but may also change other per-session settings in the future.
  4. NEWS: I have added NEWS items for these changes.
  5. documentation: I have added (at least minimally viable) documentation.
  6. Vignette(s): I've updated the introductory vignette. I used a child document to keep the README and the introductory vignette in sync, as described here https://www.garrickadenbuie.com/blog/2018/03/05/dry-vignette-and-readme/, which is something Jonathan Carroll brought up here: Vignette standards software-review-meta#55. I have not yet addressed the vectorised arguments of the geocoding functions though, as well as the other topics mentioned in Issue Vignette(s) topics #53. I propose we resume discussion about vignettes there.
  7. Issues add documentation for oc_forward and oc_reverse #34 (oc_forward docs), vignette title #54 (vignette title) and new OpenCage website URL #59 (new website URL) are fixed.
  8. Issue Before a PR asking questions here #61 is fixed, as far as safer tests are concerned. As stated there, return = "url_only" is only allowed if key = NULL or if used in an interactive session in order to prevent credential leakage.
  9. Issue keys for testing response code 402 and 403 #63, keys for testing 402/403. fixed in this commit in feat_pmap branch: dpprdan@ec344be

I'll also add the current NEWS:

opencage 0.1.4.9001

This is a major rewrite of the {opencage} package. opencage_forward() and opencage_reverse() have been deprecated and are superseded by oc_forward() and oc_reverse(), respectively. In addition there are two new functions oc_forward_df() and oc_reverse_df(), which (reverse) geocode a placename column (or latitude/longitude columns) in a data frame.

The new features include:

  • oc_forward() and oc_reverse() return either lists of data frames, JSON strings, GeoJSON strings, or URLs to be sent to the API (for debugging purposes).
  • oc_forward_df() and oc_reverse_df() take a data frame as input and return a data frame with the geocoding results, optionally with the source data frame bound to the results data frame.
  • Almost all arguments of the geocoding functions are vectorised (the exceptions being output, key and no_record), so it is possible to serially (reverse) geocode lists of placenames or coordinates. The geocoding functions show a progress indicator when more than one placename or latitude/longitude pair is provided.
  • The forward geocoding functions now support multiple countrycodes in accordance with the OpenCage API (Support multiple country codes #44). The countrycodes can now be provided in upper or lower case (Let country code be lower or upper #47).
  • A helper function oc_bbox() now makes it easier to create (lists of) bounding boxes from vectors, bbox objects and data frames.
  • http requests are now handled by {crul}, not {httr} (Ditch httr in favour of crul #37).
  • API calls are now rate limited (ratelimit API calls #32). The default limit is set to 1 call per second as per the API limit of the Free Trial plan. The rate limit can be adjusted with oc_config().

Breaking changes

  • opencage_forward(), opencage_reverse(), and opencage_key() are (soft) deprecated. opencage_key() has just been renamed to oc_key() for consistency.
  • opencage_forward() and opencage_reverse() will always output strings as characters, i.e. they won't coerce to factor depending on the stringsAsFactor option.
  • The column name for both languagecodes and countrycodes is now code, and not alpha2 and Code, respectively.

dpprdan added 30 commits April 1, 2018 21:16
…for urls with stuff like 'c(\"abbrv=1\", \"abbrv=0\")'
R/oc_forward.R Show resolved Hide resolved

results <- tidyr::unnest(results_nest, op) # nolint
# `op` is necessary, so that other list columns are not unnested
# but lintr complains about `op` not being defined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah interesting and tidyr has no such thing like dplyr's .data$?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, or rather op is part of ...?! I don't remember though if lintr would be satisfied with op being defined in globalVariables.

R/oc_forward.R Show resolved Hide resolved
R/oc_process.R Show resolved Hide resolved
R/utils.R Show resolved Hide resolved
Copy link
Member

@maelle maelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work @dpprdan, thanks a ton! I have added comments/requests but they're all minimal I think.

Two further comments

checking R code for possible problems ... NOTE
  oc_forward_df: no visible binding for global variablequeryoc_forward_df: no visible binding for global variablelatoc_forward_df: no visible binding for global variablelngoc_forward_df: no visible binding for global variableformattedoc_forward_df: no visible binding for global variableopoc_reverse_df: no visible binding for global variablequeryoc_reverse_df: no visible binding for global variableformattedoc_reverse_df: no visible binding for global variableopUndefined global functions or variables:
    formatted lat lng op query

So lintr isn't the only one to complain 😉

checking Rd cross-references ... NOTE
  Package unavailable to check Rd xrefs:sf

Please remove the cross-ref to sf because otherwise we'd need to have sf as a dependency.

dpprdan and others added 8 commits May 7, 2019 18:54
Co-Authored-By: dpprdan <possenriede@gmail.com>
Co-Authored-By: dpprdan <possenriede@gmail.com>
Co-Authored-By: dpprdan <possenriede@gmail.com>
Co-Authored-By: dpprdan <possenriede@gmail.com>
@dpprdan
Copy link
Member Author

dpprdan commented May 7, 2019

Re global variables: That is fixed in devel already. Do I need to do anything here?

Re Rd xrefs: ‘sf’ Is that from R CMD check? Why did I not see that before. It probably is from oc_bbox where one example shows to to use a sf::bbox object. Do you think we could get away with it if we wrap it in a if (requireNamespace("sf") call?

@maelle
Copy link
Member

maelle commented May 8, 2019

The cross ref is from somewhere in docs where you tried to refer to sf docs, we should not do that since sf is not a dependency. But yes to making that example safer too!

@dpprdan dpprdan merged commit 92cb65a into ropensci:devel May 8, 2019
@dpprdan dpprdan mentioned this pull request Jan 28, 2020
@dpprdan dpprdan deleted the feat_pmap branch October 29, 2020 21:05
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.

3 participants