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.

Sign up for GitHub

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

Jump to bottom

fix: allow for no gmaps_key input, add new parameters, fix .onLoad check #6

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix: allow for no gmaps_key input, add new parameters, fix .onLoad check #6

wants to merge 8 commits into from

Conversation

laresbernardo
Copy link

@laresbernardo laresbernardo commented Jul 6, 2023 •
edited
Loading

  • feat: Allow for users to use commuting_zones() without the need of fetching the coordinates using Google's API.
  • feat: Enable new parameters: quiet, longitude_col_name, latitude_col_name, …
  • fix: .onLoad warning when checking the code with devtools::check()

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 6, 2023
@gufengzhou
Copy link
Contributor

thanks a thousand:) while you're doing it already, can you also adapt the readme and the website (under website/doc/Walkthrough/Walkthrough.md), then I can deploy the website change too.

@laresbernardo laresbernardo changed the title fix: allow for no gmaps_key input fix: allow for no gmaps_key input, add new parameters, fix .onLoad check Jul 6, 2023
laresbernardo and others added 4 commits July 6, 2023 12:56
@laresbernardo
docs: add comment on gmaps_key md file
6f16fdb
@laresbernardo
docs: add comment on gmaps_key README file
b4f6138
@gufengzhou
fix: remove redundant check, quiet message, apply styler & update doc
98000d5 
- remove the redundant ggmap::has_google_key() test, because even a wrong key will return TRUE. For a wrong key, the execution will be stopped with error message when the API test ggmap::geocode("US") fails.
- use the check_gmaps_key alone to decide API key condition
- apply quiet to all messages
- styler:::style_active_pkg()
- updated documentation
@gufengzhou
fix: disable plotting when no coordinates available
026fcbf 
TSIA
@laresbernardo
Copy link
Author

LGTM!

@gufengzhou
Copy link
Contributor

gufengzhou commented Jul 10, 2023 •
edited
Loading

Currently testing the integration in GL, because it throws error when running:

GeoTestData_PreTest <- GeoDataRead(data = GeoLift_PreTest,
                                   date_id = "date",
                                   location_id = "location",
                                   Y_id = "Y",
                                   X = c(), #empty list as we have no covariates
                                   format = "yyyy-mm-dd",
                                   cluster_locations =T,
                                   find_location_lat_long = T,
                                   summary = TRUE,
                                   country_name = "United States"
                                   )

image

FYI @ArturoEsquerra @NicolasMatrices-v2 @JussanN @raphaeltamaki

NicolasMatrices-v2
NicolasMatrices-v2 suggested changes Jul 10, 2023
View reviewed changes
Copy link
Contributor

@NicolasMatrices-v2 NicolasMatrices-v2 left a comment

Choose a reason for hiding this comment

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

Bernie, could you please check the comments on made on the PR before merging? I think there are a couple of things we should edit before landing.

R/location_cluster_match.R Outdated Show resolved Hide resolved
R/find_lat_long.R Outdated Show resolved Hide resolved
@laresbernardo
Copy link
Author

@NicolasMatrices-v2 please check. Thanks for your comments and feedback. I tend to avoid stop() errors when not 100% necessary; this might be an example where that's the case.

@laresbernardo
Copy link
Author

@NicolasMatrices-v2 @gufengzhou any updates here? Are we ready to merge after the changes suggested?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasMatrices-v2 NicolasMatrices-v2 Awaiting requested review from NicolasMatrices-v2

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@laresbernardo @gufengzhou @NicolasMatrices-v2 @facebook-github-bot