-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
bikedata #116
Comments
@mpadge short question even before the real editor checks, why do you use the development version of |
That's not part of the CRAN submission (obviously), and is used because of Jim Hester's help here - see his extended commit message at top. It's temporary and will be resolved once |
Nice, thanks! |
Thanks for your submission @mpadge! I'm currently looking for reviewers. Editor checks:
Editor comments
Reviewers: @eamcvey @chucheria |
Thanks @maelle, the warnings are indeed my fault - sorry about that, and hopefully fixed already. I've no idea why |
Thanks @eamcvey @chucheria for accepting to review this package! 😺 As a reminder here are the review template and the reviewing guidelines. Your reviews are due on the 2017-06-25. @mpadge can you confirm that/when you've changed the cleanup process? (related to what @jeroen said "The cleanup script should usually only clean files generated by configure. Files generated by make should be cleaned via a clean section in Makevars" / or use cleanup.win as mentioned by Jim Hester) |
Thanks @maelle, and thanks in advance @eamcvey and @chucheria for agreeing to review - exciting to hear what you might think of it! I've done the Jim Hester |
@chucheria @eamcvey friendly reminder that your reviews are due in 10 days, on the 2017-06-25. 😉 |
Package Review
DocumentationThe package includes all the following forms of documentation:
Functionality
Estimated hours spent reviewing: 4h Review CommentsIt is a great package overall, it removes the necessity of have knowledge of many bike data APIs/file system of open data, which is a great time and effort saver. It will make more sense as more bike open data is available inside the package. Vignette has example uses and is very complete. I played with the functions a bit, it is a simple package and it allows you to use API information locally. General code commentsI found some errors executing the code. I downloaded NY data from 2016-01 to 2016-05 once I changed to Los Angeles city, it told me that all data already exist. dl_bikedata (city = 'New York City USA', dates = 201601:201605)
Downloading 201605-citibike-tripdata.zip
Downloading JC-201601-citibike-tripdata.csv.zip
Downloading JC-201602-citibike-tripdata.csv.zip
Downloading JC-201603-citibike-tripdata.csv.zip
Downloading JC-201604-citibike-tripdata.csv.zip
Downloading JC-201605-citibike-tripdata.csv.zip
dl_bikedata(city = 'la', dates = 201601:201605)
All data files already exist Once I downloaded NY data, I wanted to store it with ny <- store_bikedata (city = 'nyc', bikedb = 'bikedb', dates = 201601:201605)
Downloading data for nyc
All data files already exist
Adding data to sqlite3 database
Unzipping raw data files ...
Show Traceback
Rerun with Debug
Error in unzip(f, list = TRUE) :
zip file '/var/folders/zc/yccgnjm528j4x_665t_rz0km0000gn/T//RtmpU6AJg6/201603-citibike-tripdata.zip' cannot be opened If I do the Installation:macOS Sierra: Installation in macOS was smooth. A warning popped up in installation and in every test: Documentation:
ScriptsScripts are named with a hyphen instead of snake_case, something that rOpenSci recommends and packages seem to follow that pattern. Functions and variables are well named and explained. |
Please, tell me if I didn't explained well any of the comments or errors. |
Many thanks for your review @chucheria ! 😸 |
@eamcvey your review was due a few weeks back, will you be able to get it done soon? |
A general FYI: I had to upload a new version to CRAN because changes in the LA data were causing failures. I also addressed the first issue that @chucheria encountered so the package now gives the more informative message:
I was unable to repeat the second error, but note that |
thanks for the update @mpadge ! |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 5 Review CommentsThis is a nice package that made it easy to access the data as promised. The linked article showing analysis of this type of data is helpful to make clear what can be done with it. I was able to work through the vignette successfully and look at some additional things on my own. Some specific issues I noticed:
|
Thanks a lot for your review @eamcvey ! 😸 |
Thanks both @chucheria and @eamcvey for your reviews! 😈 I should have addressed all issues raised sometime next week, and will respond accordingly. |
Package changes in response to reviews
Changes not yet implemented
Responses to other comments not related to package changes
Thanks again @eamcvey and @chucheria for helpful insight into improving the package! |
Awesome work @mpadge! When do you think you'll be able to solve the issue mentioned in "Changes not yet implemented"? I agree reg. the authors list. @eamcvey and @chucheria are you happy with the changes? |
@mpadge For info you can already add the rOpenSci badge to the README of
|
@maelle done! |
Thanks for your comments @mpadge. I'm sorry I overlooked the About the error you commented you couldn't reproduce, I'll try to do the same thing as soon as I have some time in case it appears again an open an issue with more info in case it appears, does this sound good? |
@maelle issue#38 now done. This should resolve previous errors. Note that these are system dependent because of the different ways the bundled SQLite3 code is compiled, so it is unfortunately not possible to systematically reproduce the errors. Nevertheless, I suspect most of them arose through somehow trying to create indexes on a database that already had indexes in it. The latest modifications will robustly prevent this ever happening, and so I am pretty confident will resolve these kinds of errors. I have taken the liberty of striking through the relevant bit of my previously responses ("Changes not yet implemented") |
@mpadge ok, thanks a lot! The package looks good to me but I'll wait for @chucheria to have had time to look at the error again before approving. :-) |
absolutely - it's really important to ensure it really has been solved! |
No error, the package seems to be solid. Thanks for the patience! |
@mpadge sorry for the delay! The package is now approved, great work, and thanks @eamcvey + @chucheria for your reviews! To-dos:
Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. ((https://ropensci.org/blog/). Let me know if you are interested. |
Thanks @maelle for all of the help and guidance getting the package through to this exciting last stage!
Finally, re: blog post: absolutely interested! Yes please! |
You mean you didn't have to submit it? 🤔 Having @karthik as an editor of the R package surely helped. 😉 In my experience you need to submit it, choose Karthik as JOSS editor. Reg. the blog post let me tag @stefaniebutland who'll get in touch with you. |
okay, i've submitted it now, and put Karthik as the editor:
ping @richardellison, so you know where we're at here. new publication pending! |
I think we can close this now (I told Karthik about the JOSS submission, I check there regularly, and the blog post discussion doesn't need this to be open). Your review badge will get green! 😁 |
YIPPEE!!! 🚲 🚲 🚲 I didn't realise until I just discovered the ropensci issue on this that i should have but didn't actually request the explicit approval of @chucheria and @eamcvey for me to insert those |
Oops and I forgot to remind it to you. |
Thanks for adding me! You have my very happy approval @mpadge ! |
@mpadge (I'm responding later than I had hoped) For rOpenSci blog post you could do either a short post for the Developer Blog that has a technical focus, or do a post for the main blog whose audience is broader. Main blog post would include more narrative about your motivation for creating the package, unmet need, how-to use, good to end with a thank you to package reviewers with links to their GitHub or Twitter, point readers to issues and what you think is next to improve the package and invite people to open or address an issue etc. Deadlines:
Practical instructions:
Which type of post are you thinking of? I had been thinking of inviting you to contribute a post about Did I miss anything? |
thanks @stefaniebutland, and no worries about delayed response. I'm absolutely keen to get these packages known in any and all ways possible. Absolutely happy for at least one to be a main blog entry, for which my preference would be |
Summary
Download and aggregate data from all public hire bicycle systems which provide open data, currently including Santander Cycles in London, U.K., and from the U.S.A., citibike in New York City NY, Divvy in Chicago IL, Capital Bikeshare in Washington DC, Hubway in Boston MA, and Metro in Los Angeles LA.
https://github.com/mpadge/bikedata
Data retrieval
All those interested in analysing urban mobility - urban and general transport modellers and planners, urban scientists. Anyone wanting to make cool data visualisations of movement through some of the euro-american world's biggest cities (like this prominent example).
No current R packages enable importing of public hire bicycle data. @ramnathv has his visualising bike share repo, but that's not an R package. I've heard rumours of a package in development for live station feeds along these lines, but even if and when that appears, it would focus on real-time data (primarily bicycle availability), whereas
bikedata
focusses exclusively on archiving and aggregating historical data.Requirements
Confirm each of the following by checking the box. This package:
Publication options
paper.md
with a high-level description in the package root or ininst/
.Package will be archived as soon as the ropensci review process has been completed, in accordance with JOSS policies.
Detail
Does
R CMD check
(ordevtools::check()
) succeed? Paste and describe any errors or warnings:Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:
If this is a resubmission following rejection, please explain the change in circumstances:
If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:
@ramnathv because of his prominent prior work on analysing public hire bike systems
Note that
goodpractice
flags only:cran-comments.md
The text was updated successfully, but these errors were encountered: