Submission: tiler - Githubissues

Summary

What does this package do? (explain in 50 words or less):

Purpose: Create geographic and no-geographic map tile sets from R.

Paste the full DESCRIPTION file inside a code block below:

Package: tiler
Version: 0.2.0
Title: Create Geographic and Non-Geographic Map Tiles
Description: Creates geographic map tiles from geospatial map files or non-geographic map tiles from simple image files. This package provides a tile generator function for creating map tile sets for use with packages such as 'leaflet'. In addition to generating map tiles based on a common raster layer source, it also handles the non-geographic edge case, producing map tiles from arbitrary images. These map tiles, which have a non-geographic, simple coordinate reference system (CRS), can also be used with 'leaflet' when applying the simple CRS option. Map tiles can be created from an input file with any of the following extensions: tif, grd and nc for spatial maps and png, jpg and bmp for basic images. This package requires 'Python' and the 'gdal' library for 'Python'. 'Windows' users are recommended to install 'OSGeo4W' (<https://trac.osgeo.org/osgeo4w/>) as an easy way to obtain the required 'gdal' support for 'Python'.
Authors@R: c(person("Matthew", "Leonawicz", email = "mfleonawicz@alaska.edu", role = c("aut", "cre")),
    person("Scenarios Network for Alaska and Arctic Planning", role = c("cph", "fnd"))
    )
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
ByteCompile: true
URL: https://github.com/leonawicz/tiler
BugReports: https://github.com/leonawicz/tiler/issues
SystemRequirements: Python (>= 2.7), python-gdal library (For Windows, gdal installed via OSGeo4W <https://trac.osgeo.org/osgeo4w/> recommended)
  clipboard
Suggests:
    testthat,
    knitr,
    rmarkdown,
    lintr,
    covr,
    jpeg,
    bmp
Imports:
    sp,
    rgdal,
    raster,
    png
VignetteBuilder: knitr
RoxygenNote: 6.0.1

URL for the package (the development repository, not a stylized html page): https://github.com/leonawicz/tiler
Please indicate which category or categories from our package fit policies this package falls under *and why

geospatial, because (1) the package assists with tiling high resolution geospatial maps for practical use in online applications where displaying the source image would be too heavy a resource load and (2) even the non-geographic/"simple CRS" use case can be geospatial. For example, an image of an old, hand-drawn but historically significant map may be used as the background of a tiled map application whereas something standard like zooming in on Google Maps containing modern, current georeferenced locations would be visually incompatible and a distraction from the data shown on the map.

Who is the target audience and what are scientific applications of this package? R users who wish to create geographic as well as non-geographic map tiles, (1) easily and seamlessly with only a single line of R code (2) without a bunch of heavy package dependencies and extraneous general features and functions that do not have to do with tile generation (3) without having to code directly in other software or interact directly with Python or make calls at the command line; staying comfortably within the familiar R environment. (4) who may wish to create non-standard maps, which may also be followed by georeferencing locations in non-standard map space.

The typical applications would be to provide the generated tile sets to tile-based map applications such as Leaflet maps and this package brings together XYZ format, TMS format, and most especially the edge case of non-geographic format, together in a single interface.

Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?

Not that I am aware of on ROpenSci. The mapview package apparently has some overlapping or related functionality just added, so I think it is only in the dev version and not the CRAN version, but looking at the source code on GitHub it appears to take a different approach, piggybacking tile creation as part of a chained or piped process that creates a Leaflet map. I wanted a package that made tiles without trying to do something specific with them "on the fly", so there is no interleaving in my package of tile creation and map creation. There is definitely value in other approaches, and perhaps in the future (not during this review process, if it gets reviewed) I may add some functionality that helps streamline processes for users such as pushing their created tile sets to GitHub for serving after they've been generated, if that gives some sense of potential future package scope. But tiler is not aimed at combining tile creation with map making. They are decidedly treated as separate endeavors.

I favor the approach and perspective of separating map tile set creation (which can be slow/bulky/resource heavy depending on the map, and can create tens of thousands of tiles, or more) from applications involving the generated tiles that I'd prefer to keep remotely hosted. tiler is suited to users who want to create tile sets, host them online, and then once that processing and scaffolding is in place and ready to be served- separately do something with those tiles as base maps. While tiler contains a simple tile previewer function, this is the "extra feature", not the core purpose or functionality. The package is intended for tile generation rather than drawing maps.

If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.

Requirements

Confirm each of the following by checking the box. This package:

[x] does not violate the Terms of Service of any service it interacts with.
[x] has a CRAN and OSI accepted license.
[x] contains a README with instructions for installing the development version.
[x] includes documentation with examples for all functions.
[x] contains a vignette with examples of its essential functions and uses.
[x] has a test suite.
[x] has continuous integration, including reporting of test coverage, using services such as Travis CI, Coveralls and/or CodeCov.
[x] I agree to abide by ROpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

Publication options

[x] Do you intend for this package to go on CRAN? (This package is already on CRAN)
[ ] Do you wish to automatically submit to the Journal of Open Source Software? If so:
- [ ] The package has an obvious research application according to JOSS's definition.
- [ ] The package contains a paper.md matching JOSS's requirements with a high-level description in the package root or in inst/.
- [ ] The package is deposited in a long-term repository with the DOI:
- (Do not submit your package separately to JOSS)
[ ] Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:
- [ ] The package is novel and will be of interest to the broad readership of the journal.
- [ ] The manuscript describing the package is no longer than 3000 words.
- [ ] You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see MEE's Policy on Publishing Code)
- (Scope: Do consider MEE's Aims and Scope for your manuscript. We make no gaurantee that your manuscript willl be within MEE scope.)
- (Although not required, we strongly recommend having a full manuscript prepared when you submit here.)
- (Please do not submit your package separately to Methods in Ecology and Evolution)

Detail

[x] Does R CMD check (or devtools::check()) succeed? Paste and describe any errors or warnings:
[x] 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:

👋 @leonawicz I'm just returning from vacation (and email/work deluge). I will follow up with next steps shortly and also begin looking for reviewers. Stay tuned.

@karthik Thank you and no rush :)

Also, please feel free to ignore the unpublished (to CRAN) projections branch of the repo. Unless you think that is something assistance could be provided for. If you do consider the projections branch, it has unresolved issues, but its documentation is kept up to date with its code development. Otherwise, the master branch is what has actually been published.

Editor checks:

[x] Fit: The package meets criteria for fit and overlap
[x] Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
[x] License: The package has a CRAN or OSI accepted license
[x] Repository: The repository link resolves correctly
[ ] Archive (JOSS only, may be post-review): The repository DOI resolves correctly
[ ] Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

Editor comments

👋 @leonawicz The package looks to be in really good shape and no substantial issues emerged from the checks. The only minor suggestion is to break up long lines of code to improve readability, but we can wait to see what concrete suggestions the reviewers have.

No worries on the projections branch. Reviewers typically only address the master unless you say otherwise.

── GP tiler ────────────────────────────────────────────────────────────────────

It is good practice to

  ✖ avoid long code lines, it is bad for readability. Also,
    many people prefer editor windows that are about 80 characters
    wide. Try make your lines shorter than 80 characters

    R/tile.R:75:1
    R/tile.R:79:1
    R/tile.R:83:1
    R/tile.R:96:1
    R/tile.R:97:1
    ... and 22 more lines

You can go ahead and add a rOpenSci review badge to your readme. It will autoupdate as the review progresses.

[![](https://badges.ropensci.org/226_status.svg)](https://github.com/ropensci/onboarding/issues/226)

Reviewers: reviewer 1: @jasdumas; Currently seeking reviewer 2 Due date: reviewer 1: July 17th; TBD

@karthik Thanks! Badge added.

I can shorten the longer lines (I think my default lintr check allows 120 instead of 80). Thanks.

Yeah, please ignore the projections branch then. It is worth taking a peak at to get a sense of future scope, but I have no date set for when I would have the issues resolved and a new version out.

Thank you @jasdumas for agreeing to review! Please note due date above and reach out if you need more time. 🙏

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

[x] A statement of need clearly stating problems the software is designed to solve and its target audience in README
[x] Installation instructions: for the development version of package and any non-standard dependencies in README
[x] Vignette(s) demonstrating major functionality that runs successfully locally
[x] Function Documentation: for all exported functions in R help
[x] Examples for all exported functions in R Help that run successfully locally
[x] Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

[x] Installation: Installation succeeds as documented.
[x] Functionality: Any functional claims of the software been confirmed.
[x] Performance: Any performance claims of the software been confirmed.
[x] Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
[x] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

[x] The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 2.5 Hours

Start: 9:50 AM , End: 12:15 PM

Review Comments

Manual review of GitHub Repo, vignette & installation directions

The use of package down for the docs is great!
It would be great to see a clearer A statement of need in the README. Maybe just incorporating your comments from the review issue would be helpful
- Does this package fill a gap that leaflet does not provide (static vs. dynamic tiles)?
- Who is the intended user base?
- Also with such a visual package, it would be nice to include a image in the README of one of the resultant maps/tiles from the vignette as a teaser for GitHub.
CRAN & Development versions downloads successfully without error/messages
It would be nice to see a seperate section of linux/mac installation directions even though the last sentence of the introduction vignette states, as the only call out to Windows made it appear that this package would only work for windows users:

Linux and Mac users should not have to do any additional setup as long as Python and gdal for Python are installed.
It would be helpful to explicitly state a minimum Python 2.x or Python 3.x versions in the readme/introduction vignette, but I documented my efforts below to get the dependency gdal:
- It also seems that gdal works with python 2 using pip install GDAL
- My default python is Python 3.4.4 |Anaconda custom (x86_64)| (default, Jan 9 2016, 17:30:09)[GCC 4.2.1 (Apple Inc. build 5577)] on darwin and for python3 is Python 3.5.2 (v3.5.2:4def2a2901a5, Jun 26 2016, 10:47:25) [GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin
- I updated my system-wide python3 to 3.7 and downloaded python2.7.15 to alleviate some download pains
- I created a virtual envrionment python3 -m venv ropensci & source ropensci/bin/activate to start
- I'm still getting errors for pip3 install GDAL or pip install GDAL
- Updated setuptools: pip install --upgrade setuptools
- ok turns out that the first example in the introduction vignette works regardless of all my python mistakes :smile:
The context section is really helpful for performance considerations!

Automated tests

via opening up the Development :package: R project with packrat in a new RStudio session

devtools::check(document=FALSE):

Packages suggested but not available for checking: ‘lintr’ ‘covr’ ‘jpeg’ ‘bmp’ R CMD check results 0 errors | 0 warnings | 1 note

R CMD check succeeded

I installed install.packages(c("lintr", "covr", "jpeg", "bmp")) and the NOTE went away :tada:
devtools::test():

══ Results ════════════════════════════════════════════════════════ Duration: 9.6 s

OK: 29 Failed: 0 Warnings: 0 Skipped: 0

goodpractice::gp(): For the unit tests Goodpractice notes, i think its fine as you have some helper functions that don't need unit tests given the major functions do have unit tests. the long lines note wa already mentioned from @karthik 's editor check. The rgdal callout in the DESCRIPTION file might elude to needing a roxygen comment #' @import rgdal in one of your functions if you are leveraging a function with ::.

 ── GP tiler ───────────────────────────────────────────────────────

It is good practice to

  ✖ write unit tests for all functions, and all
    package code in general. 88% of code lines are covered
    by test cases.

    R/tile.R:79:NA
    R/tile.R:80:NA
    R/tile.R:83:NA
    R/tile.R:84:NA
    R/tile.R:87:NA
    ... and 15 more lines

  ✖ avoid long code lines, it is bad for
    readability. Also, many people prefer editor windows
    that are about 80 characters wide. Try make your lines
    shorter than 80 characters

    R/tile.R:75:1
    R/tile.R:79:1
    R/tile.R:83:1
    R/tile.R:96:1
    R/tile.R:97:1
    ... and 22 more lines

  ✖ fix this R CMD check NOTE: Namespace in
    Imports field not imported from: ‘rgdal’ All declared
    Imports should be used.

spelling::spell_check_package(): These 'mispellings' appear to just be industry nomenclature :tada:

DESCRIPTION does not contain 'Language' field. Defaulting to 'en-US'.
  WORD                 FOUND IN
Albers               tiler.Rmd:152
bmp                  tiler.Rd:16
                     description:5
CRS                  tile_viewer.Rd:14,16
                     tile.Rd:17,37,45
                     tiler.Rd:14
                     description:4
                     tiler.Rmd:19,66,82,84,98,144,148,165
EPSG                 tile.Rd:55
gdal                 description:6,7
GDAL                 tiler_options.Rd:26,28,31
georeference         tiler.Rmd:165
georeferencing       tile_viewer.Rd:18
                     tile.Rd:25
                     tiler.Rmd:179
geospatial           tile_viewer.Rd:14,16
                     tile.Rd:67
                     tiler.Rd:9
                     description:1
                     tiler.Rmd:66,84,109,144,148
geotiff              tiler.Rmd:152
grd                  tiler.Rd:16
                     description:5
https                tiler.Rd:19
                     description:7
jpg                  tiler.Rd:16
                     description:5
JS                   tile.Rd:73
                     tiler.Rmd:165
knitr                tiler.Rmd:2
nc                   tiler.Rd:16
                     description:5
osgeo                tiler.Rd:19
                     description:7
OSGeo                tiler_options.Rd:26,28
                     description:7
                     tiler.Rmd:23,23
png                  tiler.Rd:16
                     description:5
proj                 tile.Rd:39
Proj                 tile.Rd:17
QGIS                 tiler_options.Rd:28
                     tiler.Rmd:23
rasterized           tile.Rd:57
rasters              tile.Rd:62
                     tiler.Rmd:126,128
reprojected          tile.Rd:55
                     tiler.Rmd:66
reprojection         tile.Rd:27,55,56,57,58,62
                     tiler.Rmd:98,109
reprojects           tiler.Rmd:80
RGBA                 tile.Rd:58,60,61
                     tiler.Rmd:126
rmarkdown            tiler.Rmd:2
tif                  tiler.Rd:16
                     description:5
TMS                  tile.Rd:19,65,67
                     tiler.Rmd:175
trac                 tiler.Rd:19
                     description:7
unprojected          tile.Rd:61
VignetteEncoding     tiler.Rmd:2
VignetteEngine       tiler.Rmd:2
VignetteIndexEntry   tiler.Rmd:2
XYZ                  tile.Rd:19,65,68
                     tiler.Rmd:175

For extra fun, I also ran the gramr package on the pkgdown RMarkdown file, write_good_file("/Users/jasminedumas/Desktop/R-directory/ropensci_package_reviews/packrat/src/tiler/leonawicz-tiler-512c1b6/vignettes/tiler.Rmd"). Full disclosure, I developed this package at the rOpenSci Unconf17, so take the suggestions from this check with a strong grain of salt, but other than that the site is great and well written! :smile:

| index| offset|reason                                                    |
|-----:|------:|:---------------------------------------------------------|
|   530|     11|"In addition" is wordy or unneeded                        |
|   809|      7|"be used" may be passive voice                            |
|   883|     10|"be created" may be passive voice                         |
|  1067|     11|"is required" may be passive voice                        |
|  1164|      6|"simply" can weaken meaning                               |
|  1242|      5|"it is" is wordy or unneeded                              |
|  1785|      6|"is set" may be passive voice                             |
|  1815|      5|"it is" is wordy or unneeded                              |
|  1916|      4|"only" can weaken meaning                                 |
|  2121|      5|"It is" is wordy or unneeded                              |
|  2124|     14|"is recommended" may be passive voice                     |
|  2148|      6|"simply" can weaken meaning                               |
|  2372|      4|"just" can weaken meaning                                 |
|  2433|      9|"be needed" may be passive voice                          |
|  2490|     10|"additional" is wordy or unneeded                         |
|  2547|     13|"are installed" may be passive voice                      |
|  2705|      8|"There is" is unnecessary verbiage                        |
|  2820|      9|"is loaded" may be passive voice                          |
|  2939|      4|"only" can weaken meaning                                 |
|  3124|      4|"very" is a weasel word and can weaken meaning            |
|  3135|     11|"in order to" is wordy or unneeded                        |
|  3147|      8|"minimize" is wordy or unneeded                           |
|  3193|      7|"quickly" can weaken meaning                              |
|  3263|     10|"be applied" may be passive voice                         |
|  3395|      4|"very" is a weasel word and can weaken meaning            |
|  3474|     11|"a number of" is wordy or unneeded                        |
|  3615|      5|"it is" is wordy or unneeded                              |
|  3618|     14|"is recommended" may be passive voice                     |
|  3661|      4|"only" can weaken meaning                                 |
|  3848|     13|"are generated" may be passive voice                      |
|  3997|      4|"only" can weaken meaning                                 |
|  4135|     10|"subsequent" is wordy or unneeded                         |
|  4171|      8|"are used" may be passive voice                           |
|  4596|     10|"subsequent" is wordy or unneeded                         |
|  4622|      5|"it is" is wordy or unneeded                              |
|  4991|      6|"it was" is wordy or unneeded                             |
|  5168|     12|"is projected" may be passive voice                       |
|  5182|     11|"In order to" is wordy or unneeded                        |
|  5222|      7|"be used" may be passive voice                            |
|  5279|     14|"be reprojected" may be passive voice                     |
|  5325|     13|"are generated" may be passive voice                      |
|  5698|      7|"be seen" may be passive voice                            |
|  6299|      6|"likely" can weaken meaning                               |
|  6311|      9|"is wanted" may be passive voice                          |
|  6370|      5|"it is" is wordy or unneeded                              |
|  6401|     11|"was dropped" may be passive voice                        |
|  6486|     13|"are generated" may be passive voice                      |
|  7041|      6|"be set" may be passive voice                             |
|  7141|      9|"is needed" may be passive voice                          |
|  7185|      4|"just" can weaken meaning                                 |
|  7337|      6|"easily" can weaken meaning                               |
|  7344|      7|"be done" may be passive voice                            |
|  7522|     10|"are passed" may be passive voice                         |
|  7646|      7|"usually" can weaken meaning                              |
|  7654|     10|"be ignored" may be passive voice                         |
|  8187|     13|"are supported" may be passive voice                      |
|  8377|     11|"are colored" may be passive voice                        |
|  8448|      8|"prior to" is wordy or unneeded                           |
|  8523|      6|"simply" can weaken meaning                               |
|  8608|     11|"are ignored" may be passive voice                        |
|  8631|      7|"type of" is wordy or unneeded                            |
|  9159|      9|"is geared" may be passive voice                          |
|  9240|      7|"However" is wordy or unneeded                            |
|  9296|     12|"are required" may be passive voice                       |
|  9409|      7|"usually" can weaken meaning                              |
|  9609|      8|"There is" is unnecessary verbiage                        |
|  9727|      8|"are said" may be passive voice                           |
|  9813|      7|"however" is wordy or unneeded                            |
| 10051|      9|"was shown" may be passive voice                          |
| 10061|     10|"previously" can weaken meaning and is wordy or unneeded  |
| 10139|     13|"was processed" may be passive voice                      |
| 10245|      5|"It is" is wordy or unneeded                              |
| 10253|     10|"previously" can weaken meaning and is wordy or unneeded  |
| 10411|      6|"all of" is wordy or unneeded                             |
| 10428|      8|"be tiled" may be passive voice                           |
| 10711|      8|"There is" is unnecessary verbiage                        |
| 10881|      7|"be used" may be passive voice                            |
| 10965|      8|"is based" may be passive voice                           |
| 11393|     14|"is recommended" may be passive voice                     |
| 11683|      5|"It is" is wordy or unneeded                              |
| 11686|     10|"is assumed" may be passive voice                         |
| 11918|     10|"Additional" is wordy or unneeded                         |
| 12137|      4|"only" can weaken meaning                                 |
| 12400|      6|"simply" can weaken meaning                               |
| 12559|     11|"adjacent to" is wordy or unneeded                        |
| 12819|      4|"only" can weaken meaning                                 |
| 12924|      7|"Finally" can weaken meaning                              |
| 12959|     10|"additional" is wordy or unneeded                         |
| 13053|      9|"be served" may be passive voice                          |
| 13310|     11|"exclusively" can weaken meaning and is wordy or unneeded |
| 13350|      4|"just" can weaken meaning                                 |
| 13848|      8|"are used" may be passive voice                           |
| 14022|     10|"be created" may be passive voice                         |
| 14164|      6|"easily" can weaken meaning                               |
| 14171|      9|"be loaded" may be passive voice                          |
| 14340|      7|"be done" may be passive voice                            |
| 14384|      4|"just" can weaken meaning                                 |
| 14575|      7|"However" is wordy or unneeded                            |
| 14609|     10|"is created" may be passive voice                         |
| 14628|      9|"be viewed" may be passive voice                          |
| 14788|     11|"in order to" is wordy or unneeded                        |
| 15138|     15|"trial and error" is a cliche                             |

Thank you so much @jasdumas ! This is really helpful. I've included a Motivation section in the readme regarding statement of need now. I've also added an example screenshot of a tiled map.

gramr looks super cool! I can definitely use that in general. I'll try to take a closer look when I have some time.

Re: installation on different systems. I don't know how to do it for Mac and don't have access to any Macs. For Linux, the only systems I have access to are server environments that already have everything Python and geospatial installed that could possibly be needed (and I'm not the sys admin so I don't know those details).

I do use Linux with Travis CI but in that environment it's not truly testing for the successful creation of map tile files. E.g., tile can execute and not make any files as a result of system requirements not present and this is not an error. I do not know how to do this yet: Python-gdal on Travis and Appveyor

I'll take a closer look at some of the lines not covered by unit tests. Some lines can be difficult to test.

Thanks!

@leonawicz Quick update. Let me know once you've worked through Jasmine's suggestion. The second reviewers email got lost but I have contacted them again to complete the review. If it doesn't arrive shortly, I will act as second reviewer.

@karthik Thanks. I've done everything I'm able to at this time. I don't think the handful of lines without code coverage via Travis CI/Codecov are that important. Some cannot be covered like .onLoad lines (or can it?).

If ensuring a true tile-generating (i.e., file-generating) test occurs on Linux builds via Travis CI is important, I am stuck there (see link to issue above regarding installing Python and GDAL on Travis/Appveyor) and unable to resolve that myself.

Thanks @leonawicz, I'll take a look at those issues.

Also @Paula-Moraga is going to be our second reviewer and she will complete her review shortly.

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

[x] A statement of need clearly stating problems the software is designed to solve and its target audience in README
[x] Installation instructions: for the development version of package and any non-standard dependencies in README
[x] Vignette(s) demonstrating major functionality that runs successfully locally
[x] Function Documentation: for all exported functions in R help
[x] Examples for all exported functions in R Help that run successfully locally
[x] Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

[x] Installation: Installation succeeds as documented.
[x] Functionality: Any functional claims of the software been confirmed.
[x] Performance: Any performance claims of the software been confirmed.
[x] Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
[x] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

[x] The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 3 hours

Review Comments

The tiler package enables to build map tiles from geospatial and image files. The tiles created can be used with other packages such as leaflet. Some examples of tiles produced using Star Trek galaxy maps and used with leaflet are here and these are awesome!

The package works fine and the documentation is clear. The package requires Python and the gdal library for Python. This can be tricky to install for some users depending on the OS. When using Windows I needed to add some environment variables and I think these steps need to be better explained in the documentation. Also, it would be great to include in the documentation the code to create a tile and actually use it to produce a leaflet map. Other minor issues and results of the automated tests are below.

Congrats on the package!

Installation

I use Windows 10 and I needed to download and install OSGeo4W64.

To make the package work I needed to execute this: tiler_options(python = "C:/OSGeo4W64/bin/python.exe", osgeo4w = "C:/OSGeo4W64/OSGeo4W.bat")

Then I tried the package adding the Windows environment variables instead. As it is specified in the documentation, I added C:\OSGeo4W64 to the Windows environment variable PATH so R can find OSGeo4W.bat. Then I executed tile(map, tile_dir, "0-3") and I got a Warning message with status 127. I fixed that by adding C:\OSGeo4W64\bin to the Windows environment variable PATH so R can find python.exe.

Then I executed tile(map, tile_dir, "0-3") again and I got a Warning message with status 1. I fixed that by creating the environment variable PYTHONHOME with value C:\OSGeo4W64\apps\Python27/ That way R can find the local python installation path.

Documentation

I would like to see an example of the code needed to create a tile and actually use it in a leaflet map.
In section Local preview I would change project/tiles by tile_dir as in the previous examples.
I create tiles with tile() and view them with view_tiles(). I do not understand what tile_viewer() is for.
I would write the name of functions with parentheses at the end. For example, tile() instead of tile.
The documentation shows some simple examples, and says more difficult tiles can take a long time to run. I suggest you add a table with the time it takes to run different examples and the computer specifications.

Automated tests

devtools::check()

R CMD check results
0 errors | 0 warnings | 1 note 
checking package dependencies ... NOTE
Package suggested but not available for checking: 'bmp'

devtools::test()

Loading tiler
Loading required package: testthat
Testing tiler
√ | OK F W S | Context
x |  0 1 1   | lints
--------------------------------------------------------------------------------
test-lintr.R:4: warning: Package Style
incomplete final line found on 'C:/Users/moragasp/tiler-master/tiler-master/.lintr'

test-lintr.R:4: error: Package Style
Invalid DCF format.
Regular lines must have a tag.
Offending lines start with:
  inst/.lintr
1: lintr::expect_lint_free() at C:\Users\moragasp\tiler-master\tiler-master/tests/testthat/test-lintr.R:4
2: lint_package(...)
3: read_settings(path)
4: read.dcf(config_file, all = TRUE)
5: stop(gettextf("Invalid DCF format.\nRegular lines must have a tag.\nOffending lines start with:\n%s", 
       paste0("  ", lines, collapse = "\n")), domain = NA)

--------------------------------------------------------------------------------
test-tile.R:2: warning: (unknown)
package ‘raster’ was built under R version 3.4.4

test-tile.R:2: warning: (unknown)
package ‘sp’ was built under R version 3.4.4
--------------------------------------------------------------------------------
√ |  5       | viewer

== Results =====================================================================
Duration: 11.3 s

OK:       28
Failed:   1
Warnings: 3
Skipped:  0

goodpractice::gp()


It is good practice to

  ✖ write unit tests for all functions, and all package code in general. 89% of code lines are covered by test cases.

    R/tile.R:79:NA
    R/tile.R:80:NA
    R/tile.R:83:NA
    R/tile.R:84:NA
    R/tile.R:87:NA
    ... and 14 more lines

Warning messages:
1: In readLines(file) :
  incomplete final line found on 'C:/Users/moragasp/tiler-master/tiler-master/.lintr'
2: In MYPREPS[[prep]](state, quiet = quiet) : Prep step for linter failed.

spelling::spell_check_package()

DESCRIPTION does not contain 'Language' field. Defaulting to 'en-US'.

spelling::spell_check_files("README.Rmd")

  WORD             FOUND IN
AppVeyor         README.Rmd:25
codecov          README.Rmd:26
CRS              README.Rmd:34
georeferencing   README.Rmd:45
github           README.Rmd:2
gitter           README.Rmd:28
notfying         README.Rmd:64
OSGeo            README.Rmd:69
Rdoc             README.Rmd:23

@Paula-Moraga thanks for your review! :) Sorry for delays, work has been very busy lately.

The function tile_viewer is for edge cases where the preview.html file associated with a set of map tiles needs to be created because it is missing/deleted or because it must be re-created. For example, if someone runs tile multiple times with different zoom level settings and resume = TRUE, the preview.html is created every time tile is run. But the final time it is created when running title, it will not include all zoom levels. Running tile_viewer afterward with the full zoom range specified will re-create the file again, with proper full zoom level range. But no need to create tiles which already exist.

I will look into adding a visible leaflet example that demonstrates the resulting tiles soon. This is ideal, I just haven't had time to do yet. Also fixing the tile_dir specification in the Local preview section of the documentation.

I'm currently at my limit for the moment in terms of system installation knowledge/experience. I only have one Windows 10 system to test installation and use. It worked for me but I do not have the broader OS experience to be able to determine what an ideal generic solution for Windows installation that works for all users would be. I am even more limited with my knowledge about how to install system requirements on Linux at the moment, and Mac is not even in the picture for me. This is a problematic situation, but unfortunately not one I have been able to resolve on my own yet.

@leonawicz Thanks for the tile_viewer() explanation. Maybe it is useful to add this explanation in the vignette. I agree it is hard to write installation guidelines that work for everybody. Looking forward to the leaflet example!

@Paula-Moraga I still have to update the vignette with a mention about tile_viewer, but for now please see the updated vignette regarding the fun Leaflet examples. :)

I have added two examples, one geographic and one non-geographic, using leaflet to display remotely hosted tiles that were originally created with tiler. The code is shown for the original calls to tile that generated these tiles as well as the leaflet code for using them in the interactive maps shown.

These are located in the final section "Serving map tiles," in a Leaflet Examples subsection right before the Local Preview subsection. The Local Preview section is where I will also mention tile_viewer when I get to that next. Let me know what you think. Thanks!

@leonawicz The leaflet examples are not working for me. Could it be that the variables tiles <- "https://leonawicz.github.io/tiles/us48lr/tiles/{z}/{x}/{y}.png" and tiles <- "https://leonawicz.github.io/tiles/st2/tiles/{z}/{x}/{y}.png" are not right?

I have been checking here after using pkgdown and hosting online with the GitHub repo: https://leonawicz.github.io/tiler/articles/tiler.html

The Leaflet widgets in the tiler.html file are also displaying for me locally.

Are you seeing something different?

Oh wait, I think I understand. If you mean the code is not working in RStudio, click the little button at the top of the map viewing pane to open the widget in your browser. Then it may display. Alternatively, run the code in regular R (not RStudio). This should also work automatically. Let me know if this fixes it. I don't know the details, but remotely hosted components may not display automatically in RStudio's preview pane.

Yes, that was the reason! I do not see them in RStudio's preview pane but when I click the right panel it works perfectly! The examples you chose are great, thank you!

@leonawicz 👋 I just wanted to check in on the status of this review. Have all of the issues raised by @Paula-Moraga been addressed now?

@karthik @Paula-Moraga I think so??? I've done everything I planned on doing

There are some other issues I mentioned earlier that I am unable to do. See https://github.com/ropensci/onboarding/issues/226#issuecomment-414060516 for example of something I really can't figure out, but don't know how critical it is- I defer to your judgement on the necessity. https://github.com/leonawicz/tiler/issues/6

Side note: I pushed a small Windows-only bug fix to CRAN last week which is pending (v0.2.1).

Thanks @leonawicz! I see that all issues have now been addressed. I'll proceed with accepting your package.

Congrats @leonawicz, your submission has been approved! 🎉 Thank you for submitting and @jasdumas and @Paula-Moraga for thorough and timely reviews. To-dos:

[x] Transfer the repo to the rOpenSci organization under "Settings" in your repo. I have invited you to a team that should allow you to do so. You'll be made admin once you do.
[x] Add the rOpenSci footer to the bottom of your README

[![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)

[ ] Fix any links in badges for CI and coverage to point to the ropensci URL. (We'll turn on the services on our end as needed)

Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/technotes/) or long-form post with more narrative about its development. ((https://ropensci.org/blog/). If you are, @stefaniebutland will be in touch about content and timing.

@karthik @jasdumas @Paula-Moraga Thank you all for your help throughout! :)

I will find time in the next day to complete the documentation updates and complete the transfer.

Congrats, @leonawicz!

Hello @leonawicz and congratulations. This link will give you many examples of blog posts by authors of onboarded packages. In case you are considering writing one, here are some technical and editorial guidelines: https://github.com/ropensci/roweb2#contributing-a-blog-post.

No obligation to do this. Happy to answer any questions.

ropensci / software-review

Submission: tiler #226

Summary

Requirements

Publication options

Detail

Editor checks:

Editor comments

Package Review

Documentation

Functionality

Final approval (post-review)

Start: 9:50 AM , End: 12:15 PM

Review Comments

Manual review of GitHub Repo, vignette & installation directions

Automated tests

Package Review

Documentation

Functionality

Final approval (post-review)

Review Comments

Installation

Documentation

Automated tests