Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
osmplotr onboarding request #27
[added by @richfitz:
In short: This is the only package that allows OpenStreetMap data to be presented in a visually customisable way.
On Fri, Mar 25, 2016 at 9:36 PM, Scott Chamberlain <email@example.com
Jeff W. Hollister
At long last, here is my review @mpadge!
In addition, I had some other general comments to think about.
rOpenSci Package Guidelines:
The comments below refer directly to the rOpenSci package guidelines. I've made suggested changes where appropriate.
I've gone through each of the functions below and provided comments on documentation, functionality, ease of use, etc. Where possible I have made comments about the code itself, but given the size of the code base in
Thanks a lot for the impressively detailed and extremely helpful review @jhollist . I agree with pretty much every issue you raised, and will attend to them all in a revised version asap. Before doing that, I'd appreciate it if you could let me know what you think about one general question regarding the abiding purpose of the package. I think many of the issues you picked up on reflect the fact that I initially designed the package to provide a simple way to plot OSM data, but modified this purpose along the way to provide data visualisation tools (primarily with
You claim that the ability
and that you
This is maybe your only claim with which I disagree. I think the
This latter function was added after writing the vignettes. Please accept my apology for not including it there, but the reason was that I envision re-structuring the vignettes, and thought it better to wait for initial comments before doing that. I propose to ditch the
Your comments also helped me realise how I might overcome what I see as one big issue in that the package relies on the admittedly unorthodox requirement of manually closing devices at the end of map construction. This could easily be avoided by constructing maps as
Note also that I avoided using classes in the code as it stands, because the only direct benefit would be in
I'll address all of the other issues you raised in the meantime, including tests, internalising functions, ensuring projections are kept throughout, reducing dependencies. I'll let you know when the code is ready for further perusal.
... and one final comment:
@mpadge Couple of quick thoughts for you.
A function that just provides an easy interface to dev.copy and shuts down that device should do the trick.
Look forward to seeing the revisions!
Thanks again @jhollist for the extremely helpful review. Below you'll find my detailed responses to all of your concerns. But first a couple of general points for you to note regarding the changes to
Test suite added. It's not yet complete, but well on the way, and remains (in my mind) the only major task.
All functions now have examples, and many of them include code for downloading data. Examples were a bit thin in the previous version because Uwe Ligges explicitly told me to remove the
Done. Important improvement: thanks!
The relatively long time taken for my response has been because I have rejigged everything to
Thanks for the tip! You were right: I had neglected to attach projections to objects returned from
Reference removed. Thanks.
I've not yet changed this for one reason in particular. Future development will require re-writing several
rOpenSci Package Guidelines:
Done exactly as suggested: It's not yet complete, but a good start.
I've done my best to remove as many dependencies as possible, and believe that it is now down to a minimal necessary number
Example updated and greatly extended to hopefully make it far clearer and more relevant. It's still called
Extensive example now given that hopefully clarifies the primary usage, but perhaps more importantly, a huge portion of new vignette ('making-maps-with-data') devoted to this function.
I must admit I have not greatly changed the description of
All suggestions implemented. Previous versions did not have
The entire function has simply been ditched, because it was unlikely to have been particularly useful anyway.
Extensive examples now provide use case and, hopefully, broader context.
All purely internal functions now.
Extensive examples now included. Warnings now handled at the point of
This is a good suggestion, but not yet implemented because I was focussing on other things (
Examples provided. Error fixed - again, please accept my apologies for that one!
Not any more.
See my response above to
... that last comment was the best of all your suggestions. Thank you for the enormous improvements this has made to almost all functionality of the package!
It's been incorporated within 'making-maps'.
There are now two vignettes, 'making-maps' and 'making-maps-with-data'. The second of these focusses entirely on
Looking forward to your response to this enormously improved version of
Look forward to digging back into this. Swamped this week, but may have
On Wed, Apr 27, 2016 at 6:08 AM, mark padgham firstname.lastname@example.org
Jeff W. Hollister
fyi @jhollist : I uploaded to CRAN and found out that vignette names are cut to some maximal length, so that 'making-maps' and 'making-maps-with-data' ended up with same names. README still links to those CRAN vignettes, but names are now changed in /vignettes.
Thanks for taking the time to address my comments in the first review. I think it is improved over the first draft. That being said, I still see two distinct use cases for your package: the wrapper to the Overpass API and the viz tools for the resultant
After re-reading the fit criteria, I feel
I can see where this might fit with rOpenSci, but also can see where it doesn't. This isn't meant to be a critique per se, just that the fit is not an obvious one for me and I think others should make that call.
While I do like the
Scaling of the map
The map scaling isn't working correctly, as the plot fills the graphic device instead of being constrained to the ratio of the bounding box. You have examples where you set the width and height of the output image, but that relies on users to set proper height and width. Since this information is part of getting the data in the first place, relying on additional work by the users isn't necessary.
If I were using the package I'd most likely do something along the lines of (with some obvious clean up needed):
Your ggplot code is using coord_cartesian() which is scaling the axes relative to the device size. coord_equal() is forcing the change in y to be the same as the change in x. While technically incorrect for unprojected coordinates, it is a better option, IMO, than having the axes scaled to the device. The change in x and y is close to being equal for most areas mapped by OSM (poles, not so much).
A couple of thoughts on the vignettes
If you have any questions or concerns about my review, let me know.
... to which my general response is that I think that the primary advantage of the package lies in the ability to use OSM data themselves for scientific visualisation. This ability--as I believe I have made clear in the second vignette along with the main git osmplotr readme--is both assuredly scientific, and is in my opinion not readily available using any other current package. @jhollist doubts that he,
yet I think that
that I believe the combinations of the second vignette, the git osmplotr readme, and the maps included below convincingly demonstrate the direct scientific utility of
I therefore agree with his final comment that,
and so, @sckott, let's do this.
A general response
The primary advance of
I reiterate my firm convictions that:
The concerns of @jhollist seem to be that aspects of the underlying functionality can be readily reproduced directly wtih
Finally, such simple maps are simply an auxiliary utility beside the main functionality of being able to visualise user-provided data using actual map objects rather than just overlaying layers of visually arbitrary data upon pre-generated maps. Maps such as this:
can surely not be readily reproduced using any other package, and would surely be all but impossible for anyone not deeply conversant with
Once again @jhollist, your comments are perceptive and helpful, and will assuredly be addressed soon, but it's perhaps better for the moment to get that second pair of eyes on the case.
Just to be clear, I do think this is a nice addition (especially the API access). Hopefully my second response didn't come of as overly harsh! And, @mpadge I do take your point that for those wanting to map OSM data in R but who aren't already familiar with ggplot2 this would be an improvement.
Were this a journal review, I think I'd be on the "Accept, with some reservations" side of things.
Again, thanks for you work on this and also for your thoughtful responses!
ggplot2 Implementation / Scaling of the map
Previous usage of
This only affects the first vignette, which I have now rewritten to illustrate the two ways of saving maps to file. I personally prefer direct device calls, because
New names implemented, but I avoided calling the first one 'Intro' because (i) I would interpret that to indicate that it described the major functionality, and (ii) it would not be clear how it related to the second vignette. I believe that the chosen names ('basic-maps' and 'data-maps') are more indicative both of the content of these two vignettes and of the relationship between them.
There were some inactive links within the documents which are now all active. (There are also links to CRAN versions of the re-named vignettes which will only work after CRAN re-submission.)
Done - thanks!
Happy to respond to any further suggestions or issues, and even happier to look forward to imminent transfer of