submission: rsat #437

unai-perez · 2021-03-21T22:09:19Z

Date accepted: 2021-09-30
Submitting Author: Unai Pérez-Goya (@unai-perez)
Other Authors: Manuel Montesino-SanMartin (@mmontesinosanmartin), Ana F Militino (@militino), María Dolores Ugarte (@lolaugartemartinez)
Repository: https://github.com/spatialstatisticsupna/rsat
Version submitted: 0.1.14
Editor: @jhollist
Reviewers: @khondula, @mhweber

Due date for @khondula: 2021-05-25

Due date for @mhweber: 2021-06-23
Archive: TBD
Version accepted: TBD

Paste the full DESCRIPTION file inside a code block below:

Package: rsat
Type: Package
Title: Tools for Downloading, Customizing, and Processing Time Series of Satellite Images from Landsat, MODIS, and Sentinel 
Version: 0.1.14
Author: U Pérez - Goya [aut, cre] <unai.perez@unavarra.es>,
        M Montesino - SanMartin [aut] <manuel.montesino@unavarra.es>,
        A F Militino [aut] <militino@unavarra.es>,
        M D Ugarte [aut] <lola@unavarra.es>
Maintainer: U Perez - Goya <unai.perez@unavarra.es>
Description: Downloading, customizing, and processing time series of satellite images for a region of interest. 'rsat' functions allow a unified access to multispectral images from Landsat, MODIS and Sentinel repositories. 'rsat' also offers capabilities for customizing satellite images, such as tile mosaicking, image cropping and new variables computation. Finally, 'rsat' covers the processing, including cloud masking, compositing and gap-filling/smoothing time series of images (Militino et al., 2018 <doi:10.3390/rs10030398> and Militino et al., 2019 <doi:10.1109/TGRS.2019.2904193>).
Depends: R (>= 3.5.0), raster, sf, stars
Imports: XML, curl, httr, leafem, leaflet, rjson, rvest, tmap, xml2, zip, methods, sp, Rdpack, fields, calendR
RdMacros: Rdpack
Suggests: 
    rgdal,
    knitr,
    rmarkdown,
    covr,
    testthat
License: GPL-3
Encoding: UTF-8
LazyData: true
RoxygenNote: 7.1.1
Collate: 
    'add2rtoi.R'
    'api.R'
    'extent_crs.R'
    'records.R'
    'rtoi.R'
    'cloud_mask.R'
    'connections.R'
    'data.R'
    'derive.R'
    'download.R'
    'list_data.R'
    'mosaic.R'
    'mosaic_fun_SYN.R'
    'mosaic_fun_ls.R'
    'mosaic_fun_mod.R'
    'mosaic_fun_sen2.R'
    'mosaic_generic.R'
    'package_tools.R'
    'plot.R'
    'preview.R'
    'search_mod.R'
    'search_ls.R'
    'search_sen.R'
    'search.R'
    'smoothing_images.R'
    'variables.R'
VignetteBuilder: knitr

Scope

Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
- data retrieval
- data extraction
- data munging
- data deposition
- workflow automation
- version control
- citation management and bibliometrics
- scientific software wrappers
- field and lab reproducibility tools
- database software bindings
- geospatial data
- text analysis
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
The package is focuses on searching, downloading, and preprocessing imagery data from Landsat, Modis, and Sentinel. It also include procedures for deriving variables and cloud filling.
Who is the target audience and what are scientific applications of this package?
Anyone interested in Remote Sensing or researchers looking for satellite imagery data.
Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?
Nothing that is functionally similar. Many source-specific packages exist, but none that aggregate across sources.

The most similar package could be MODIStsp. But the package only contemplates the use of Modis satellite images, while 'rsat' is focuses on the standardization and homogenization of data between different satellite programs. 'rsat' supports Modis, Landsat and Sentinel data, handling multi platform data in a database and optimizing its processing.

(If applicable) Does your package comply with our guidance around Ethics, Data Privacy and Human Subjects Research?
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.

Technical checks

Confirm each of the following by checking the box.

I have read the guide for authors and rOpenSci packaging guide.

This package:

does not violate the Terms of Service of any service it interacts with.
has a CRAN and OSI accepted license.
contains a README with instructions for installing the development version.
includes documentation with examples for all functions, created with roxygen2.
contains a vignette with examples of its essential functions and uses.
has a test suite.
has continuous integration, including reporting of test coverage using services such as Travis CI, Coveralls and/or CodeCov.

Publication options

Do you intend for this package to go on CRAN?
Do you intend for this package to go on Bioconductor?
Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:

MEE Options

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 guarantee that your manuscript will 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)

Code of conduct

I agree to abide by rOpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

The text was updated successfully, but these errors were encountered:

noamross · 2021-03-23T18:30:25Z

@ropensci-review-bot assign @jhollist as editor

ropensci-review-bot · 2021-03-23T18:30:54Z

Assigned! @jhollist is now the editor

jhollist · 2021-03-23T18:45:41Z

@noamross Thanks for setting me up!

@unai-perez Just wanted to let you know that I am new to this editor role. So bare with me as I figure things out. Also, my next week is pretty booked. I'll start digging in where I can, but it'll be first of April before I will be able to make real progress on coordinating the review.

Any questions, let me know.

unai-perez · 2021-03-24T12:02:06Z

Thank for the heads-up @jhollist. Looking forward to receiving your comments and the comments from the reviewers. We will both learn along the way.

jhollist · 2021-04-08T14:23:58Z

@unai-perez Thanks for your patience. I finally was able to get some time to run through this (and learn what needed to be done!). See below for my comments. Let me know if you have questions.

Editor checks:

Documentation: The package has sufficient documentation available online (README, pkgdown docs) to allow for an assessment of functionality and scope without installing the package.
Fit: The package meets criteria for fit and overlap
Automated tests: Package has a testing suite and is tested via a CI service.
License: The package has a CRAN or OSI accepted license
Repository: The repository link resolves correctly

Editor comments

This looks to be very useful package and builds nicely on existing functionality. It fits well with the ROpenSci criteria. Installation proceeds as expected from both local and GitHub sources. During my checks I did find a number of items that need to be addressed before we can move along with the review process. Pay close attention to https://devguide.ropensci.org/building.html as you revise your submission but see also my comments below for a detailed list.

If you have any questions, let me know. Items marked with a * will need to be addressed prior to assigning reviewers. Items without a * may still be raised as part of the review process, but the review can proceed prior to addressing those.

Lastly, I do want to reiterate that I think this will be a very useful package. Having a one stop shop for gathering and preparing satellite data is needed and kudos for taking that on!

Automated Tests

*Expand CI to include Windows, Linux, and Mac and using the latest, previous and development versions of R. You are currently using appveyor which is using the latest version and Windows only. You may want to look at GitHub Actions which is supported via usethis::use_github_action_check_standard(). This will test on all the needed platforms and releases.

Masked function names

I did notice a few masked function names (e.g. rsat::subset). I agree that masking is sometimes hard to avoid, but it might be worthwhile to rename is possible. It is likely that reviewers might catch this as well.

Checks and Tests

Tests (e.g. devtools::check() and devtools::test()) are currently failing on my machine, plus I am seeing several warnings.

*Check warnings on tests. Some might be acceptable but others may need
correction. I see:

-- Warning (test-search.R:17:5): search test -----------------------------------
Error in Earth Explorer api connection. HTTP 404.
Backtrace:
 1. rsat::sat_search(...) test-search.R:17:4
 2. rsat::sat_search(...)
 3. rsat:::.local(region, product, ...)
 5. rsat:::ls_search(region(region), product = s, ...)
 6. rsat:::.local(region, product, ...)
-- Warning (test-search.R:17:5): search test -----------------------------------
Error in Earth Explorer api connection. HTTP 404.
Backtrace:
 1. rsat::sat_search(...) test-search.R:17:4
 2. rsat::sat_search(...)
 3. rsat:::.local(region, product, ...)
 5. rsat:::ls_search(region(region), product = s, ...)
 6. rsat:::.local(region, product, ...)
-- Warning (test-search.R:17:5): search test -----------------------------------
Error in Earth Explorer api connection. HTTP 404.
Backtrace:
 1. rsat::sat_search(...) test-search.R:17:4
 2. rsat::sat_search(...)
 3. rsat:::.local(region, product, ...)
 5. rsat:::ls_search(region(region), product = s, ...)
 6. rsat:::.local(region, product, ...)
-- Warning (test-search.R:17:5): search test -----------------------------------
Error in Earth Explorer api connection. HTTP 404.
Backtrace:
 1. rsat::sat_search(...) test-search.R:17:4
 2. rsat::sat_search(...)
 3. rsat:::.local(region, product, ...)
 5. rsat:::ls_search(region(region), product = s, ...)
 6. rsat:::.local(region, product, ...)
-- Warning (test-search.R:17:5): search test -----------------------------------
Error in Earth Explorer api connection. HTTP 404.
Backtrace:
 1. rsat::sat_search(...) test-search.R:17:4
 2. rsat::sat_search(...)
 3. rsat:::.local(region, product, ...)
 5. rsat:::ls_search(region(region), product = s, ...)
 6. rsat:::.local(region, product, ...)
-- Warning (test-search.R:17:5): search test -----------------------------------
Error in Earth Explorer api connection. HTTP 404.
Backtrace:
 1. rsat::sat_search(...) test-search.R:17:4
 2. rsat::sat_search(...)
 3. rsat:::.local(region, product, ...)
 5. rsat:::ls_search(region(region), product = s, ...)
 6. rsat:::.local(region, product, ...)
-- Warning (test-search.R:17:5): search test -----------------------------------
Cannot perform Landsat search, check your credentials and/or the api status: https://m2m.cr.usgs.gov/api/docs/json/
Backtrace:
 1. rsat::sat_search(...) test-search.R:17:4
 2. rsat::sat_search(...)
 3. rsat:::.local(region, product, ...)
 5. rsat:::ls_search(region(region), product = s, ...)
 6. rsat:::.local(region, product, ...)
-- Warning (test-search.R:35:5): search test -----------------------------------
GDAL Error 1: PROJ: proj_create_from_database: crs not found
Backtrace:
 1. base::plot(c(rcds1, rcds2, rcds3)) test-search.R:35:4
 2. rsat::plot(c(rcds1, rcds2, rcds3))
 3. rsat:::.local(x, y, ...)
 5. sf:::st_crs.numeric(crs(r))
 6. sf:::make_crs(paste0("EPSG:", x))
 7. sf:::CPL_crs_from_input(x)

*Make sure tests pass. I see:

== Failed tests ================================================================
-- Error (test-search.R:35:5): search test -------------------------------------
Error: attempt to select less than one element in get1index
Backtrace:
    x
 1. +-base::plot(c(rcds1, rcds2, rcds3)) test-search.R:35:4
 2. \-rsat::plot(c(rcds1, rcds2, rcds3))
 3.   \-rsat:::.local(x, y, ...)
 4.     \-connection$getApi(get_api_name(r))

Goodpractice

Many issues are raised with goodpractice::gp(). Pay close attention to the output and fix these issues. In particular:

Consider simplifying genPlotGIS

  ✖ write short and simple functions. These functions have high
    cyclomatic complexity:genPlotGIS (60).

*You currently have sf, raster, and stars as Depends. Those can all be moved to Imports. You will need to make sure to import those (e.g. with @importFrom) where you use them in your functions.

  ✖ not use "Depends" in DESCRIPTION, as it can cause name
    clashes, and poor interaction with other packages. Use "Imports"
    instead.

*Add a URL field. Prior to acceptance as an ROpenSci package this would be https://github.com/spatialstatisticsupna/rsat.

  ✖ add a "URL" field to DESCRIPTION. It helps users find
    information about your package online. If your package does not have a
    homepage, add an URL to GitHub, or the CRAN package package page.

*Add BugReports field. Prior to acceptance as an ROpenSci package this would be https://github.com/spatialstatisticsupna/rsat/issues.

  ✖ add a "BugReports" field to DESCRIPTION, and point it to a bug
    tracker. Many online code hosting services provide bug trackers for
    free, https://github.com, https://gitlab.com, etc.

*It appears you are using both '=' and '<-' for assignment. The ROpenSci style guide allows either, but you need to be consistent. Please choose one and use it throughout your code.

  ✖ use '<-' for assignment instead of '='. '<-' is the standard,
    and R users and developers are used it and it is easier to read your
    code for them if you use '<-'.

    R\api.R:21:21
    R\api.R:22:21
    R\api.R:23:20
    R\api.R:24:20
    R\api.R:25:24
    ... and 102 more lines

Work on getting your long lines shortened. The styler package will help with some of these, but not all (e.g. Enforcing a 80-character line length rule? r-lib/styler#247). You will have to manually edit many of these. You can use lintr::lint(path/to/file.R) to help find these. Also, you can turn on an 80 character guide in RStudio with Tools:Global Options:Code:Display and select Show Margin and set characters to 80. This will help visually identify these 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\api.R:6:1
    R\api.R:118:1
    R\api.R:120:1
    R\api.R:133:1
    R\api.R:150:1
    ... and 321 more lines

*Omit any trailing semicolons.

  ✖ omit trailing semicolons from code lines. They are not needed
    and most R coding standards forbid them

    R\mosaic.R:166:92
    R\preview.R:217:40

*Replace sapply() with vapply()

  ✖ avoid sapply(), it is not type safe. It might return a vector,
    or a list, depending on the input data. Consider using vapply() instead.

    R\package_tools.R:40:10
    R\search_ls.R:107:17

*When building a sequence use seq_len() or seq_along()

  ✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...), 1:NROW(...) and
    1:NCOL(...) expressions. They are error prone and result 1:0 if the
    expression on the right hand side is zero. Use seq_len() or seq_along()
    instead.

    R\download.R:74:22
    R\download.R:107:24
    R\download.R:125:24
    R\mosaic_generic.R:61:103
    R\package_tools.R:42:12
    ... and 9 more lines

*Refine importing of functions. Instead of @import, use @importFrom package function_name

  ✖ do not import packages as a whole, as this can cause name clashes
    between the imported packages. Instead, import only the specific
    functions you need.

*Same testing error as mentioned above.

  ✖ checking tests ... Running 'testthat.R' ERROR Running the
    tests in 'tests/testthat.R' failed. Last 13 lines of output: Data and
    variable methods provided by rsat Satellite products: ls1, ls2, ls3,
    ls4, ls5, ls7, ls8, mod09ga, myd09ga, mcd43a4, Sentinel-1, Sentinel-2,
    Sentinel-3, SY_2_SYN___. Variable Methods: EVI, MSAVI2, NBR, NBR2, NDMI,
    NDVI, NDWI, RGB, SAVI.== Failed tests
    ================================================================ --
    Error (test-search.R:35:5): search test
    ------------------------------------- Error: attempt to select less than
    one element in get1index Backtrace: x 1. +-base::plot(c(rcds1, rcds2,
    rcds3)) test-search.R:35:4 2. \-rsat::plot(c(rcds1, rcds2, rcds3)) 3.
    \-rsat:::.local(x, y, ...) 4.  \-connection$getApi(get_api_name(r)) [
    FAIL 1 | WARN 8 | SKIP 0 | PASS 0 ] Error: Test failures Execution
    halted

I only see this one with goodpractice::gp(), not when I run check on its own. You can probably ignore this one.

  ✖ fix this R CMD check WARNING: LaTeX errors when creating PDF
    version. This typically indicates Rd problems. LaTeX errors found:

*Switch T and F to TRUE and FALSE

  ✖ avoid 'T' and 'F', as they are just variables which are set to
    the logicals 'TRUE' and 'FALSE' by default, but are not reserved words
    and hence can be overwritten by the user.  Hence, one should always use
    'TRUE' and 'FALSE' for the logicals.

    R/api.R:NA:NA
    R/cloud_mask.R:NA:NA
    R/cloud_mask.R:NA:NA
    R/derive.R:NA:NA
    R/derive.R:NA:NA
    ... and 19 more lines

Spelling

Several items are flagged with devtools::spell_check() but most appear to be names, acronyms, etc. and are probably not really misspellings. Please double check the items returned by devtools::spell_check() to make sure any true misspellings are corrected.

Tests

*Expand tests: Since I had the one test fail locally, I wasn't able to confirm coverage. On your README, you report 54% and it appears that many of the exported functions in your package are not covered by your current tests. I would like to see the testing expanded to 70% or greater. I do realize that testing with API packages can be a challenge sometimes, so if reaching 70% is a problem you will need to provide a justification. Additionally, if authentication or API keys are required you need to include instructions in your documentation (e.g. in README). In short, if there are any prerequisites for your tests to pass, makes sure users and reviewers can easily set those up on their local machines.

unai-perez · 2021-04-09T17:05:24Z

@jhollist thanks for your comments.

I think we can deal with most of the changes in a short period of time. I think the biggest change is the testing. We will try to get to the 70% that you mention.

I was wondering if I could send the current version of the package to CRAN. It would be great for us to report that for a project. However, we are concerned about the change of the name of the package during the ropensci revision. Therefore, we would like to receive your advice on this regard.

jhollist · 2021-04-09T17:58:20Z

Sounds good. Let me get some other opinions on the pre-review CRAN submission. Stay tuned.

…

On Fri, Apr 9, 2021 at 1:05 PM Unai Pérez-Goya ***@***.***> wrote: @jhollist <https://github.com/jhollist> thanks for your comments. I think we can deal with most of the changes in a short period of time. I think the biggest change is the testing. We will try to get to the 70% that you mention. I was wondering if I could send the current version of the package to CRAN. It would be great for us to report that for a project. However, we are concerned about the change of the name of the package during the ropensci revision. Therefore, we would like to receive your advice on this regard. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#437 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJPYSYZCZK7SVJO4ILYSRLTH4XWRANCNFSM4ZSADESA> .

-- Jeffrey W. Hollister email: ***@***.*** cell: 401 556 4087 https://jwhollister.com

jhollist · 2021-04-09T18:17:03Z

@unai-perez, the decision to submit to CRAN is ultimately going to be up to you and your co-authors. That being said the ROpenSci recommendation is to to wait until after review because of the possibility of changes to your package's interface and also because the reviewers will often find problems that could complicate the CRAN submission process.

I would like to encourage you to wait a bit on the submission, if you can. I already have a few reviewers in mind and will do my best to speed that part of the process along.

As I said, the decision is yours to make, but I think waiting until after the reviews come in will be an easier path in the long run.

unai-perez · 2021-04-27T15:42:34Z

@jhollist thanks for your comments. We decided not to send the package to CRAN until the review process is completed.
We covered most of the requirements. Find below the line by line answers.

Automated Tests

* [x]  `*`Expand CI to include Windows, Linux, and Mac and using the latest, previous and development versions of R.  You are currently using appveyor which is using the latest version and Windows only.  You may want to look at GitHub Actions which is supported via `usethis::use_github_action_check_standard()`.  This will test on all the needed platforms and releases.

Done, we integrated the package in GitHub Actions. Additionally, we change the minimum R version supported by the package to 4.0.0. Packages we depend on have experienced major changes since the 4.0.0. R version, which means that rsat is no longer backward compatible for earlier versions.

Masked function names

* [ ]  I did notice a few masked function names (e.g. `rsat::subset`).  I agree that masking is sometimes hard to avoid, but it might be worthwhile to rename is possible.  It is likely that reviewers might catch this as well.

These masked functions are developed to add functionalities to the new classes created by the package. For example, rsat::subset gets a subset from records, that works as vector.
We created new methods for these functions, so they accept rtois or records as inputs. When used with other classes, their original functionalities remain the same. We think that this strategy benefits R users as they do not need to remember new names specific for rtois/records.

Checks and Tests

Tests (e.g. devtools::check() and devtools::test()) are currently failing on my machine, plus I am seeing several warnings.

* [x]  `*`Check warnings on tests.  Some might be acceptable but others may need
  correction.  I see:

Done. Now only a few warnings related to modis image projections are displayed

-- Warning (test-search.R:17:5): search test -----------------------------------
Error in Earth Explorer api connection. HTTP 404.
Backtrace:
 1. rsat::sat_search(...) test-search.R:17:4
 2. rsat::sat_search(...)
 3. rsat:::.local(region, product, ...)
 5. rsat:::ls_search(region(region), product = s, ...)
 6. rsat:::.local(region, product, ...)
-- Warning (test-search.R:17:5): search test -----------------------------------
Error in Earth Explorer api connection. HTTP 404.
Backtrace:
 1. rsat::sat_search(...) test-search.R:17:4
 2. rsat::sat_search(...)
 3. rsat:::.local(region, product, ...)
 5. rsat:::ls_search(region(region), product = s, ...)
 6. rsat:::.local(region, product, ...)
-- Warning (test-search.R:17:5): search test -----------------------------------
Error in Earth Explorer api connection. HTTP 404.
Backtrace:
 1. rsat::sat_search(...) test-search.R:17:4
 2. rsat::sat_search(...)
 3. rsat:::.local(region, product, ...)
 5. rsat:::ls_search(region(region), product = s, ...)
 6. rsat:::.local(region, product, ...)
-- Warning (test-search.R:17:5): search test -----------------------------------
Error in Earth Explorer api connection. HTTP 404.
Backtrace:
 1. rsat::sat_search(...) test-search.R:17:4
 2. rsat::sat_search(...)
 3. rsat:::.local(region, product, ...)
 5. rsat:::ls_search(region(region), product = s, ...)
 6. rsat:::.local(region, product, ...)
-- Warning (test-search.R:17:5): search test -----------------------------------
Error in Earth Explorer api connection. HTTP 404.
Backtrace:
 1. rsat::sat_search(...) test-search.R:17:4
 2. rsat::sat_search(...)
 3. rsat:::.local(region, product, ...)
 5. rsat:::ls_search(region(region), product = s, ...)
 6. rsat:::.local(region, product, ...)
-- Warning (test-search.R:17:5): search test -----------------------------------
Error in Earth Explorer api connection. HTTP 404.
Backtrace:
 1. rsat::sat_search(...) test-search.R:17:4
 2. rsat::sat_search(...)
 3. rsat:::.local(region, product, ...)
 5. rsat:::ls_search(region(region), product = s, ...)
 6. rsat:::.local(region, product, ...)
-- Warning (test-search.R:17:5): search test -----------------------------------
Cannot perform Landsat search, check your credentials and/or the api status: https://m2m.cr.usgs.gov/api/docs/json/
Backtrace:
 1. rsat::sat_search(...) test-search.R:17:4
 2. rsat::sat_search(...)
 3. rsat:::.local(region, product, ...)
 5. rsat:::ls_search(region(region), product = s, ...)
 6. rsat:::.local(region, product, ...)
-- Warning (test-search.R:35:5): search test -----------------------------------
GDAL Error 1: PROJ: proj_create_from_database: crs not found
Backtrace:
 1. base::plot(c(rcds1, rcds2, rcds3)) test-search.R:35:4
 2. rsat::plot(c(rcds1, rcds2, rcds3))
 3. rsat:::.local(x, y, ...)
 5. sf:::st_crs.numeric(crs(r))
 6. sf:::make_crs(paste0("EPSG:", x))
 7. sf:::CPL_crs_from_input(x)

* [x]  `*`Make sure tests pass.  I see:

Done.

== Failed tests ================================================================
-- Error (test-search.R:35:5): search test -------------------------------------
Error: attempt to select less than one element in get1index
Backtrace:
    x
 1. +-base::plot(c(rcds1, rcds2, rcds3)) test-search.R:35:4
 2. \-rsat::plot(c(rcds1, rcds2, rcds3))
 3.   \-rsat:::.local(x, y, ...)
 4.     \-connection$getApi(get_api_name(r))

Goodpractice

Many issues are raised with goodpractice::gp(). Pay close attention to the output and fix these issues. In particular:

* [x]  Consider simplifying `genPlotGIS`

Done. The function was divided into smaller functions.

  ✖ write short and simple functions. These functions have high
    cyclomatic complexity:genPlotGIS (60).

* [x]  `*`You currently have sf, raster, and stars as Depends.  Those can all be moved to Imports.  You will need to make sure to import those (e.g. with @importFrom) where you use them in your functions.

Done. All the packages as Depends were passed to imports. Now the package only uses importFrom.

  ✖ not use "Depends" in DESCRIPTION, as it can cause name
    clashes, and poor interaction with other packages. Use "Imports"
    instead.

* [x]  `*`Add a URL field.  Prior to acceptance as an ROpenSci package this would be https://github.com/spatialstatisticsupna/rsat.

Done.

  ✖ add a "URL" field to DESCRIPTION. It helps users find
    information about your package online. If your package does not have a
    homepage, add an URL to GitHub, or the CRAN package package page.

* [x]  `*`Add BugReports field.  Prior to acceptance as an ROpenSci package this would be https://github.com/spatialstatisticsupna/rsat/issues.

Done.

  ✖ add a "BugReports" field to DESCRIPTION, and point it to a bug
    tracker. Many online code hosting services provide bug trackers for
    free, https://github.com, https://gitlab.com, etc.

* [x]  `*`It appears you are using both '=' and '<-' for assignment.  The ROpenSci style guide allows either, but you need to be consistent.  Please choose one and use it throughout your code.

Done.

  ✖ use '<-' for assignment instead of '='. '<-' is the standard,
    and R users and developers are used it and it is easier to read your
    code for them if you use '<-'.

    R\api.R:21:21
    R\api.R:22:21
    R\api.R:23:20
    R\api.R:24:20
    R\api.R:25:24
    ... and 102 more lines

* [x]  Work on getting your long lines shortened.  The `styler` package will help with some of these, but not all (e.g. [r-lib/styler#247](https://github.com/r-lib/styler/issues/247)).  You will have to manually edit many of these.  You can use `lintr::lint(path/to/file.R)` to help find these.  Also, you can turn on an 80 character guide in RStudio with Tools:Global Options:Code:Display and select Show Margin and set characters to 80.  This will help visually identify these lines.

Done. There are some lines with more than 80 characters, but these are URLs that it is not possible to cut.

  ✖ 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\api.R:6:1
    R\api.R:118:1
    R\api.R:120:1
    R\api.R:133:1
    R\api.R:150:1
    ... and 321 more lines

* [x]  `*`Omit any trailing semicolons.

Done. We fixed this.

  ✖ omit trailing semicolons from code lines. They are not needed
    and most R coding standards forbid them

    R\mosaic.R:166:92
    R\preview.R:217:40

* [x]  `*`Replace `sapply()` with `vapply()`

Done. Now the package uses vapply.

  ✖ avoid sapply(), it is not type safe. It might return a vector,
    or a list, depending on the input data. Consider using vapply() instead.

    R\package_tools.R:40:10
    R\search_ls.R:107:17

* [x]  `*`When building a sequence use `seq_len()` or `seq_along()`

Done. Now the package uses seq_len or seq_along.

  ✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...), 1:NROW(...) and
    1:NCOL(...) expressions. They are error prone and result 1:0 if the
    expression on the right hand side is zero. Use seq_len() or seq_along()
    instead.

    R\download.R:74:22
    R\download.R:107:24
    R\download.R:125:24
    R\mosaic_generic.R:61:103
    R\package_tools.R:42:12
    ... and 9 more lines

* [x]  `*`Refine importing of functions.  Instead of @import, use @importFrom package function_name

Done, now we only use importFrom.

  ✖ do not import packages as a whole, as this can cause name clashes
    between the imported packages. Instead, import only the specific
    functions you need.

* [x]  `*`Same testing error as mentioned above.

Fixed.

  ✖ checking tests ... Running 'testthat.R' ERROR Running the
    tests in 'tests/testthat.R' failed. Last 13 lines of output: Data and
    variable methods provided by rsat Satellite products: ls1, ls2, ls3,
    ls4, ls5, ls7, ls8, mod09ga, myd09ga, mcd43a4, Sentinel-1, Sentinel-2,
    Sentinel-3, SY_2_SYN___. Variable Methods: EVI, MSAVI2, NBR, NBR2, NDMI,
    NDVI, NDWI, RGB, SAVI.== Failed tests
    ================================================================ --
    Error (test-search.R:35:5): search test
    ------------------------------------- Error: attempt to select less than
    one element in get1index Backtrace: x 1. +-base::plot(c(rcds1, rcds2,
    rcds3)) test-search.R:35:4 2. \-rsat::plot(c(rcds1, rcds2, rcds3)) 3.
    \-rsat:::.local(x, y, ...) 4.  \-connection$getApi(get_api_name(r)) [
    FAIL 1 | WARN 8 | SKIP 0 | PASS 0 ] Error: Test failures Execution
    halted

* [x]  I only see this one with `goodpractice::gp()`, not when I run check on its own.  You can probably ignore this one.

Running goodpractice::gc(), there is no warning anymore.

  ✖ fix this R CMD check WARNING: LaTeX errors when creating PDF
    version. This typically indicates Rd problems. LaTeX errors found:

* [x]  `*`Switch T and F to TRUE and FALSE

Fixed! We changed all T and F as TRUE and FALSE.

  ✖ avoid 'T' and 'F', as they are just variables which are set to
    the logicals 'TRUE' and 'FALSE' by default, but are not reserved words
    and hence can be overwritten by the user.  Hence, one should always use
    'TRUE' and 'FALSE' for the logicals.

    R/api.R:NA:NA
    R/cloud_mask.R:NA:NA
    R/cloud_mask.R:NA:NA
    R/derive.R:NA:NA
    R/derive.R:NA:NA
    ... and 19 more lines

Spelling

* [x]  Several items are flagged with `devtools::spell_check()` but most appear to be names, acronyms, etc. and are probably not really misspellings.  Please double check the items returned by `devtools::spell_check()` to make sure any true misspellings are corrected.

We check all the misspellings

Tests

* [x]  `*`Expand tests: Since I had the one test fail locally, I wasn't able to confirm coverage.  On your README, you report 54% and it appears that many of the exported functions in your package are not covered by your current tests.  I would like to see the testing expanded to 70% or greater.  I do realize that testing with API packages can be a challenge sometimes, so if reaching 70% is a problem you will need to provide a justification.  Additionally, if authentication or API keys are required you need to include instructions in your documentation (e.g. in README).  In short, if there are any prerequisites for your tests to pass, makes sure users and reviewers can easily set those up on their local machines.

Testing APIs and large datasets is challenging and time consuming. We propose an alternative solution to meet this requirement while keeping it simple and computationally fast. We added a new argument called test.mode in some functions. This argument allows to run a few more lines of code to speed-up the testing process and avoid very specific errors that very difficult to anticipate (e.g., omitting calls to the API and replacing them with calls to GitHub mimicking the API response). In addition, we added a new function to test internal methods of the package, which are usually omitted by the test, such as functions designed to order images, etc... With these changes we have managed to exceed the 70% of lines executed in codecov.

maelle · 2021-05-03T06:23:03Z

@ropensci-review-bot assign @jhollist as editor

ropensci-review-bot · 2021-05-03T06:23:05Z

Assigned! @jhollist is now the editor

jhollist · 2021-05-03T17:51:17Z

@unai-perez Thanks for addressing these issues. I think we are very close.

I have one last thing that needs to be addressed prior to starting review. The tests, as currently constructed, don't finish on my machine. I dug into it a little and it looks like lines 59-70 in test-download.r (https://github.com/spatialstatisticsupna/rsat/blob/381d12de6d974eee0e99e0b41f7fd2931c0b6c15/tests/testthat/test-download.R#L59-L70) are the source of the problem. If I comment those out and re-run the tests everything completes in about 20 minutes on my machine. These do appear to run on CI (at least GitHub Actions didn't throw any errors) so that is a little weird. Full disclosure, I didn't spend too much time trying find the underlying issue.

I would like you and your co-authors to do the following:

See if you can recreate this issue with the most recent version on https://github.com/spatialstatisticsupna/rsat. I cloned this version locally and the ran devtools::check("rsat"). It ran overnight before I stopped it. I think if you let it run an hour or so and it hasn't completed then that indicates an issue with that code.
If you recreate it, please try to find the issue and correct it. If the tests run fine for you, then I am happy to move on with the review process.

Any questions or concerns about this, just let me know.

unai-perez · 2021-05-04T15:19:03Z

Dear @jhollist,
After testing lines 59-70, we find a problem. In these lines we download images from the scihub API, but it seems that since last Wednesday they are migrating the scihub image server.
https://scihub.copernicus.eu/news/News00865
https://scihub.copernicus.eu/news/News00866

There is an error with those images at the moment and the API shows that they are not available. The package interprets that images will be available soon and waits for the images to be ready. However, this is not the case and the process gets stuck.

We have decided to remove those lines from the test and keep only the lines in test.mode=TRUE.

jhollist · 2021-05-04T18:11:33Z

Ahh! The fun of API packages.

Sounds like this is something that won't be a long-term issue and once fixed you can add that test back in. I think holding that particular test back is a good short-term solution and we can move forward with the review.

jhollist · 2021-05-04T18:12:18Z

@ropensci-review-bot seeking reviewers

ropensci-review-bot · 2021-05-04T18:12:19Z

Please add this badge to the README of your package repository:

[![Status at rOpenSci Software Peer Review](https://badges.ropensci.org/437_status.svg)](https://github.com/ropensci/software-review/issues/437)

Furthermore, if your package does not have a NEWS.md file yet, please create one to capture the changes made during the review process. See https://devguide.ropensci.org/releasing.html#news

jhollist · 2021-05-04T19:44:14Z

@ropensci-review-bot add @khondula to reviewers

ropensci-review-bot · 2021-05-04T19:44:25Z

@khondula added to the reviewers list. Review due date is 2021-05-25. Thanks @khondula for accepting to review! Please refer to our reviewer guide.

jhollist · 2021-05-04T19:45:17Z

@khondula As we discussed via email, if you need additional time on the review, just let me know.

jhollist · 2021-05-17T18:37:54Z

@unai-perez Just wanted to give you a quick update and let you know that I haven't forgotten!

As you can see in the thread, we have one reviewer. I am working on getting a second.

jhollist · 2021-06-02T16:47:35Z

@ropensci-review-bot add @mhweber to reviewers

ropensci-review-bot · 2021-06-02T16:47:37Z

@mhweber added to the reviewers list. Review due date is 2021-06-23. Thanks @mhweber for accepting to review!

jhollist · 2021-06-02T16:49:35Z

@ropensci-review-bot reviewers assigned

mhweber · 2021-06-22T00:12:04Z

@jhollist , @unai-perez, here is my review of rsat. Overall a really nice useful package and a great addition - details below. Main issue in my review was the Earth Explorer api which still seemed to be having issues in my testing. I left two boxes unchecked below due to this api issue and suggestion of adding contributing to README.

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

Briefly describe any working relationship you have (had) with the package authors.
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:

A statement of need clearly stating problems the software is designed to solve and its target audience in README
Installation instructions: for the development version of package and any non-standard dependencies in README
Vignette(s) demonstrating major functionality that runs successfully locally
Function Documentation: for all exported functions
Examples (that run successfully locally) for all exported functions
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

Installation: Installation succeeds as documented.
Functionality: Any functional claims of the software been confirmed.
Performance: Any performance claims of the software been confirmed.
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.
Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Estimated hours spent reviewing: 10

Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

A useful and well-documented package
Please add contributing guidelines to README
Tests currently have 1 Fail and 64 Warnings:
- Main issue appears to be the Earth Explorer api connection - tests generate HTTP 404 error. I had same thing happen playing with my own searches using the package and my NASA Earth Explorer username and password.
- Aside from the api connection issue, numerous Proj4 warnings in testing
- Error I believe due to rtoi trying to use Landsat-8 with failed Earth Explorer api connection
I like the use of band names in rsat rather than band number to provide generic use of unique custom functions for different satellites / missions.
Are there user interface improvements that could be made?
- It was a bit confusing in vignette how provided built-in variable such as NDVI, NBR, or NDSI should be used w/out a custom function, since the example only builds a custom function rather than using one of the built-ins.
- Minor detail but it would seem more intuitive for the order of parameters in subset to be: records object, name of the slot, value of the slot, as opposed to value of slot before name of slot.
- It would be nice to see direct example in vignette of download a simple records object, rather than just the examples shown of downloading for an rtoi or using records method on rtoi.
- I would love to see functions prefixed with something like 'sat_' rather than use of some very generic function names within the package such as download
Warning message for derive has typo: message(paste0("Product not mosaicked, mosaic the product ",
"from you will derive variables."))
Should have 'from which you will derive'
One suggestion that I've seen used in another package, nhdplusTools that would be nice for this one would be to add a section for related similar packages- there's a growing ecosystem of remote sensing R packages and would be nice to see how this package fits into / complements that ecosystem - I'm thinking of rgee, RSToolbox, OpenImageR and others.

khondula · 2021-06-22T21:02:13Z

Hi @jhollist and @unai-perez , please find my review below.

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

Briefly describe any working relationship you have (had) with the package authors.
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:

A statement of need clearly stating problems the software is designed to solve and its target audience in README
Installation instructions: for the development version of package and any non-standard dependencies in README
Vignette(s) demonstrating major functionality that runs successfully locally
Function Documentation: for all exported functions
Examples (that run successfully locally) for all exported functions
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

Installation: Installation succeeds as documented.
Functionality: Any functional claims of the software been confirmed.
Performance: Any performance claims of the software been confirmed.
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.
Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Estimated hours spent reviewing:

Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

Overall comments

This package has nice functionality to help work with satellite data. I was able to reproduce the functionality in most of the vignettes and examples, but outside of the vignettes I found it somewhat difficult to navigate the documentation. This could be improved with more adherence to ropensci's packaging guidelines about function documentation (e.g. should be more obvious to find what an rtoi object is). Second, given the numerous other packages that have overlapping functionality (listed below), the documentation should describe how this package compares whether it is compatible (e.g. can you convert between objects with class rtoi and satellite?). Additionally, there are several functions where warnings are produced based on underlying changes to rspatial packages (ie. phase out of proj4strings), which may affect functionality in the future. I'd recommend revising to address the warnings and if possible add support for terra in addition to or in favor of raster package.

Specific comments

Code of conduct, contribution guidelines, and top-level documentation to return from ?rsat are missing and suggested by rOpenSci package guidelines
Please provide information about how rsat compares/relates to packages with overlapping functionality including: RGISTools, rLandsat, getSpatialData, getlandsat, sen2r, MODIS, luna, satellite, landsat, sits
The log-in profiles section of the readme is somewhat confusing. The instructions recommend having the same user name and password for both services, with "username is 4 characters long and includes a period, number or underscore". I already had existing accounts with a username that did not meet this criteria, and wasn't able to use the package with those credentials. I was able to use the package by setting the credentials used in the vignette.
Vignette(s) worked locally, but would be helpful to have links to rendered versions in the readme. It could also help navigate package functionality to include numbers with the logical sequence of workflow (01_search, 02_download, 03_customize, 04_process). There is also inconsistent language for these 4 verbs between vignettes, comments in documentation (readme comment # search, acquire, customize, and process), and function names (acquire vs download, derive vs process). Renaming and/or using object_verb format could help reduce conflicts with other packages and make package more in line with ropensci guidelines
There are several references in vignettes where only the identifier is given not a full citation ([@ndsi2004], [@mvc1986], [@ima2019])
The plot function at end of example in the readme did not work, seems to require adding "preview" as second argument. (The first suggestion in the error message (adding y = "view") also gave an error.)

> plot(navarre, "mod09ga", variable = "NDVI", breaks = seq(0, 1, 0.1))
Error in .local(x, y, ...) : 
  plot needs a parameter 'y' with one of the following values: 'view', 'preview', or 'dates'.
> plot(navarre, y = "view", "mod09ga", variable = "NDVI", breaks = seq(0, 1, 0.1))
Error in .local(x, y, ...) : view mode requires product argument.

function names - several functions names are in conflict with other packages: derive (ggplot2), rename (dplyr), download (downloader). Consider renaming functions with the object_verb style recommended by ropensci (rsat_download or rtoi_download?)
Majority of exported functions do not have @return or @examples in documentation, and of the examples most are set to not run. Only a few of the functions have the level of detail that allows for 'multiple points of entry' in terms of providing enough context to understand function inputs and outputs. Many of the helper functions are somewhat straightforward in terms of what they do, but it was hard to determine their utility without more explanation in the documentation. Adding top-level package documentation and potentially grouping functions (as suggested by @mhweber ) could go a long way towards improving documentation for this package.
In addition to clarifying return value from sat_search in documentation, it may be helpful to return messages from sat_search about objects that are added to the rtoi (similar to message if there are no images resulting from the search).
The examples included for several functions are the same workflow ending at slightly different points (derive, get_raster, get_stars, download, etc.). It would be helpful to include more than one variation of functions in the examples to demonstrate possible variations (like is done for sat_search). Additionally, the example workflow takes a relatively long time to run and may encounter timeout errors - is it possible to demonstrate the same functionality with fewer dates/images?
The example for smoothing_images uses spplot and stack which are in packages not otherwise loaded (sp and raster). spplot produces warnings associated with upgrades in rspatial infrastructure.

Warning messages:
1: In wkt(obj) : CRS object has no comment

The example for rename doesn't run as included
following example in sat_search doesn't run as included.

> navarre <- new_rtoi(
+   "Navarre", # name of the region
+   ex.navarre, # sf of the region
+   rtoi.path
+ ) 
Error in (function (classes, fdef, mtable)  : 
  unable to find an inherited method for function ‘new_rtoi’ for signature ‘"character", "sf", "character", "missing", "missing", "missing"’

warning produced for example in mosaic:

Warning message:
In CPL_gdalbuildvrt(source, destination, options, oo, quiet) :
  GDAL Message 1: VSIFSeekL(xxx, SEEK_END) may be really slow on GZip streams.

Other comments

package name - The name is short and descriptive, however when searching "rsat" there are many results for "remote server administrator tools" and RSATtools, a BioConductor package for interfacing with genomics "regulatory sequence analysis tools". This package does usually come up on the first page of results if "rsat r" is specified (but not for a more generic search like "rsat help"). I think the name works, maybe there are ways for rOpenSci to help with search engine optimization.
Tested on macOS (10.15.7). No problems and devtools::check() ran successfully without any errors/warnings/notes.

jhollist · 2021-06-29T14:44:25Z

@unai-perez Sorry for late notice on this (on vacation last week!) but both of the reviewers have submitted their reviews. If you have any questions about next steps let me know. Look forward to your responses.

jhollist · 2021-07-13T16:58:40Z

@unai-perez Just checking in on this. Been about three weeks since the last review came in. Let me know if you have any questions about the process or the reviews.

unai-perez · 2021-07-16T08:30:54Z

Dear @hollis

We are working on the main comments from the reviewers; however, these changes require several checks before publication as bugs may arise. The main lines we are addressing are:

function renaming: these changes make it easier to improve some references in the manual but checking the whole package after every change to make sure we don't add new bugs is time consuming.
improving the use of the package: In this line we improve the examples in the manual and add messages in methods to give recommendations on how to use the functions.
comparison with other packages: as @khondula indicates we are going to add a small comparison between several packages in which we are going to omit RGISTools since it is of our authorship and we are going to discontinue this one to promote rsat.

In addition, we have detected some problems with the latest versions of R and its dependencies. For example, @khondula has detected some tests that take a long time to finish. Those examples should take only a few seconds, at least the searching commands. Some of the coding used in the modis image search, although it works correctly, should not take more than 1 second to give the result. We have detected that it is a problem that appears with the new version of R and we are solving it.

As soon as we have all these changes made and we will comment one by one all the recommendations.

jhollist · 2021-07-16T14:14:09Z

@unai-perez Sounds like a good plan. I'll check on your progress in a few weeks.

jhollist · 2021-08-10T17:56:32Z

@unai-perez Just checking in to see how things are going with your response. If there is anything you need on my end, just let me know.

unai-perez · 2021-09-09T10:14:46Z

Dear @jhollist, @khondula and @mhweber. We have finally answered all the comments you have made. The most important and costly question has been the use of terra instead of raster. Now the raster package is only used in a few exceptional code pieces. The user manual has been substantially improved, especially the examples that now allow unitary tests without having to show the whole package workflow over and over again.
We have added a new database path management, which simplifies the rtoi and the usability of the package. The way an rtoi is stored has also been changed and improved. Some functions have better performance now and small bugs have been fixed throughout the package.
We believe that all these changes have substantially improved the package and can be better fitted in future enhancements.
In the following lines I address each comment and response to the reviewers recommendation:
Reviewer 1

@jhollist , @unai-perez, here is my review of rsat. Overall a really nice useful package and a great addition - details below. Main issue in my review was the Earth Explorer api which still seemed to be having issues in my testing. I left two boxes unchecked below due to this api issue and suggestion of adding contributing to README.

Thank you very much, we think your comments have been very accurate and we have implemented all of them. We have corrected the issues and added the contributing section.

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
• Briefly describe any working relationship you have (had) with the package authors.
• 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:
• A statement of need clearly stating problems the software is designed to solve and its target audience in README
• Installation instructions: for the development version of package and any non-standard dependencies in README
• Vignette(s) demonstrating major functionality that runs successfully locally
• Function Documentation: for all exported functions
• Examples (that run successfully locally) for all exported functions
• 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
• Installation: Installation succeeds as documented.
• Functionality: Any functional claims of the software been confirmed.
• Performance: Any performance claims of the software been confirmed.
• 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.
• Packaging guidelines: The package conforms to the rOpenSci packaging guidelines
Estimated hours spent reviewing: 10
• Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments
• A useful and well-documented package
• Please add contributing guidelines to README
• Tests currently have 1 Fail and 64 Warnings:
o Main issue appears to be the Earth Explorer api connection - tests generate HTTP 404 error. I had same thing happen playing with my own searches using the package and my NASA Earth Explorer username and password.
o Aside from the api connection issue, numerous Proj4 warnings in testing
o Error I believe due to rtoi trying to use Landsat-8 with failed Earth Explorer api connection

Fixed! this does not happen anymore

• I like the use of band names in rsat rather than band number to provide generic use of unique custom functions for different satellites / missions.
• Are there user interface improvements that could be made?
o It was a bit confusing in vignette how provided built-in variable such as NDVI, NBR, or NDSI should be used w/out a custom function, since the example only builds a custom function rather than using one of the built-ins.

DONE. We have added new examples that generate different indexes. Some of them are build-in and others are user defined.

o Minor detail but it would seem more intuitive for the order of parameters in subset to be: records object, name of the slot, value of the slot, as opposed to value of slot before name of slot.

DONE. We have decided to change the order of these parameters.

o It would be nice to see direct example in vignette of download a simple records object, rather than just the examples shown of downloading for an rtoi or using records method on rtoi.

DONE. We have added an example of downloading some records.

o I would love to see functions prefixed with something like 'sat_' rather than use of some very generic function names within the package such as download

DONE. The most important functions in the package now are named as “rsat_”

• Warning message for derive has typo: message(paste0("Product not mosaicked, mosaic the product ",
"from you will derive variables."))
Should have 'from which you will derive'

DONE. We fixed this typo

• One suggestion that I've seen used in another package, nhdplusTools that would be nice for this one would be to add a section for related similar packages- there's a growing ecosystem of remote sensing R packages and would be nice to see how this package fits into / complements that ecosystem - I'm thinking of rgee, RSToolbox, OpenImageR and others.

DONE. We have added this section with a little remote sensing package comparison.

Thank you very much for your helpful comments.

Reviewer 2
Hi @jhollist and @unai-perez , please find my review below.
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
• Briefly describe any working relationship you have (had) with the package authors.
• 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:
• A statement of need clearly stating problems the software is designed to solve and its target audience in README
• Installation instructions: for the development version of package and any non-standard dependencies in README
• Vignette(s) demonstrating major functionality that runs successfully locally
• Function Documentation: for all exported functions
• Examples (that run successfully locally) for all exported functions
• 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
• Installation: Installation succeeds as documented.
• Functionality: Any functional claims of the software been confirmed.
• Performance: Any performance claims of the software been confirmed.
• 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.
• Packaging guidelines: The package conforms to the rOpenSci packaging guidelines
Estimated hours spent reviewing:
• Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments
Overall comments
This package has nice functionality to help work with satellite data. I was able to reproduce the functionality in most of the vignettes and examples, but outside of the vignettes I found it somewhat difficult to navigate the documentation. This could be improved with more adherence to ropensci's packaging guidelines about function documentation (e.g. should be more obvious to find what an rtoi object is). Second, given the numerous other packages that have overlapping functionality (listed below), the documentation should describe how this package compares whether it is compatible (e.g. can you convert between objects with class rtoi and satellite?). Additionally, there are several functions where warnings are produced based on underlying changes to rspatial packages (ie. phase out of proj4strings), which may affect functionality in the future. I'd recommend revising to address the warnings and if possible add support for terra in addition to or in favor of raster package.

Thank you very much for your review, we have followed your indications and we have recoded the package to use terra. We have also revised the entire manual and improved all the entries to make the package easier to use.

Specific comments
• Code of conduct, contribution guidelines, and top-level documentation to return from ?rsat are missing and suggested by rOpenSci package guidelines

DONE. We added this section to the package

• Please provide information about how rsat compares/relates to packages with overlapping functionality including: RGISTools, rLandsat, getSpatialData, getlandsat, sen2r, MODIS, luna, satellite, landsat, sits

DONE. We added a comparison in the readme.

• The log-in profiles section of the readme is somewhat confusing. The instructions recommend having the same user name and password for both services, with "username is 4 characters long and includes a period, number or underscore". I already had existing accounts with a username that did not meet this criteria, and wasn't able to use the package with those credentials. I was able to use the package by setting the credentials used in the vignette.

DONE. We added how to create a user account that was missing in the readme

• Vignette(s) worked locally, but would be helpful to have links to rendered versions in the readme. It could also help navigate package functionality to include numbers with the logical sequence of workflow (01_search, 02_download, 03_customize, 04_process). There is also inconsistent language for these 4 verbs between vignettes, comments in documentation (readme comment # search, acquire, customize, and process), and function names (acquire vs download, derive vs process). Renaming and/or using object_verb format could help reduce conflicts with other packages and make package more in line with ropensci guidelines

DONE. We rename the vignettes and we add a link in the rsat section of the manual

• There are several references in vignettes where only the identifier is given not a full citation ([@ndsi2004], [@mvc1986], [@ima2019])

DONE. We fixed these references.

• The plot function at end of example in the readme did not work, seems to require adding "preview" as second argument. (The first suggestion in the error message (adding y = "view") also gave an error.)

DONE. We rewrite all the examples in plot to ease its usability

plot(navarre, "mod09ga", variable = "NDVI", breaks = seq(0, 1, 0.1))
Error in .local(x, y, ...) :
plot needs a parameter 'y' with one of the following values: 'view', 'preview', or 'dates'.
plot(navarre, y = "view", "mod09ga", variable = "NDVI", breaks = seq(0, 1, 0.1))
Error in .local(x, y, ...) : view mode requires product argument.
• function names - several functions names are in conflict with other packages: derive (ggplot2), rename (dplyr), download (downloader). Consider renaming functions with the object_verb style recommended by ropensci (rsat_download or rtoi_download?)

DONE. Now we use 'rsat_’ prefix in the main functions

• Majority of exported functions do not have @return or @examples in documentation, and of the examples most are set to not run. Only a few of the functions have the level of detail that allows for 'multiple points of entry' in terms of providing enough context to understand function inputs and outputs. Many of the helper functions are somewhat straightforward in terms of what they do, but it was hard to determine their utility without more explanation in the documentation. Adding top-level package documentation and potentially grouping functions (as suggested by @mhweber ) could go a long way towards improving documentation for this package.

DONE. We redefine all the examples to show the use of the package

• In addition to clarifying return value from sat_search in documentation, it may be helpful to return messages from sat_search about objects that are added to the rtoi (similar to message if there are no images resulting from the search).

DONE. The function now prints the number of records found in each API.

• The examples included for several functions are the same workflow ending at slightly different points (derive, get_raster, get_stars, download, etc.). It would be helpful to include more than one variation of functions in the examples to demonstrate possible variations (like is done for sat_search). Additionally, the example workflow takes a relatively long time to run and may encounter timeout errors - is it possible to demonstrate the same functionality with fewer dates/images?

DONE. Now the example shows more variations of these functions and we add a new function rsat_get_spatRaster to get terra raster class

• The example for smoothing_images uses spplot and stack which are in packages not otherwise loaded (sp and raster). spplot produces warnings associated with upgrades in rspatial infrastructure.

DONE. We redefine the function to run IMA using terra package and show its different user

Warning messages:
1: In wkt(obj) : CRS object has no comment
• The example for rename doesn't run as included
We have not been able to replicate the error, it could be that some library on your computer kept an image open and lock the folder name. We are following up to see if we can fix it in next commits.
• following example in sat_search doesn't run as included.

FIXED! this does not happen anymore

navarre <- new_rtoi(

"Navarre", # name of the region

ex.navarre, # sf of the region

rtoi.path

)
Error in (function (classes, fdef, mtable) :
unable to find an inherited method for function ‘new_rtoi’ for signature ‘"character", "sf", "character", "missing", "missing", "missing"’
• warning produced for example in mosaic:
Warning message:
In CPL_gdalbuildvrt(source, destination, options, oo, quiet) :
GDAL Message 1: VSIFSeekL(xxx, SEEK_END) may be really slow on GZip streams.

FIXED! this does not happen anymore

Other comments
• package name - The name is short and descriptive, however when searching "rsat" there are many results for "remote server administrator tools" and RSATtools, a BioConductor package for interfacing with genomics "regulatory sequence analysis tools". This package does usually come up on the first page of results if "rsat r" is specified (but not for a more generic search like "rsat help"). I think the name works, maybe there are ways for rOpenSci to help with search engine optimization.
• Tested on macOS (10.15.7). No problems and devtools::check() ran successfully without any errors/warnings/notes.

Thank you very much for your helpful comments.

jhollist · 2021-09-15T23:35:39Z

@unai-perez thank you for getting these edits in!

@khondula and @mhweber could you take a look at the edits an the comments above and indicate that your comments have been addressed to your approval/satisfaction?

And sorry for the delay in responding to your edits. We are almost there!

mhweber · 2021-09-22T22:31:33Z

@jhollist , @unai-perez my comments and suggestions have all been addressed - appreciate the work in making these updates!

jhollist · 2021-09-29T19:11:50Z

Thanks, @mhweber!

jhollist · 2021-09-29T19:12:40Z

@khondula Do these edits address the suggestions in your review?

khondula · 2021-09-29T19:52:53Z

Wow, impressive work @unai-perez!
I'm afraid I'm missing something - I am no longer able to run the vignettes and can't figure out why?

library(rsat)
browseVignettes(package = "rsat")
No vignettes found by browseVignettes(package = "rsat")
vignette("rsat1_search", package = "rsat")
Warning message:
vignette ‘rsat1_search’ not found

unai-perez · 2021-09-30T07:47:46Z

@khondula, I have just checked the vignettes and they are right. Maybe you have installed the package without the argument "build_vignettes=TRUE".

Anyway, I have changed the installation guide for installing "rmarkdown" in case it is not already installed, and to compile the vignettes by default.

Thank you very much for your comments.

jhollist · 2021-09-30T11:42:51Z

I can confirm that with install_github("spatialstatisticsupna/rsat", build_vignettes = TRUE) I see the 4 vignettes.

khondula · 2021-09-30T13:56:22Z

Thank you, that fixed it!

The example in the readme workflow produces an error when I get to the first plot - is it possible that making the toi a range of 2 dates avoids this?

> plot(navarre, "view" , 
+      product = "mod09ga", 
+      variable = "NDVI", 
+      breaks = seq(0, 1, 0.1))
Error in if (dx[i] < dd[i]) { : missing value where TRUE/FALSE needed

I think everything else I checked looks good. The additional documentation really improves usability!

unai-perez · 2021-09-30T14:32:52Z

@khondula, you're right! I fixed that bug. Now, the single images are displayed correctly with plot.

khondula · 2021-09-30T14:45:23Z

@jhollist, @unai-perez has addressed everything from my review

jhollist · 2021-09-30T18:10:39Z

@ropensci-review-bot approve rsat

ropensci-review-bot · 2021-09-30T18:10:41Z

Approved! Thanks @unai-perez for submitting and @khondula, @mhweber for your reviews! 😁

To-dos:

Transfer the repo to rOpenSci's "ropensci" GitHub 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.
Fix all links to the GitHub repo to point to the repo under the ropensci organization.
Delete your current code of conduct file if you had one since rOpenSci's default one will apply, see https://devguide.ropensci.org/collaboration.html#coc-file
If you already had a pkgdown website and are ok relying only on rOpenSci central docs building and branding,
- deactivate the automatic deployment you might have set up
- remove styling tweaks from your pkgdown config but keep that config file
- replace the whole current pkgdown website with a redirecting page
- replace your package docs URL with https://docs.ropensci.org/package_name
- In addition, in your DESCRIPTION file, include the docs link in the URL field alongside the link to the GitHub repository, e.g.: URL: https://docs.ropensci.org/foobar (website) https://github.com/ropensci/foobar
Fix any links in badges for CI and coverage to point to the new repository URL.
Please check you have updated the package version to a post-review version and that you documented all changes in NEWS.md
We're starting to roll out software metadata files to all rOpenSci packages via the Codemeta initiative, see https://docs.ropensci.org/codemetar/for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent).

Welcome aboard! We'd love to host a post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions.

We maintain an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved.

Last but not least, you can volunteer as a reviewer via filling a short form.

jhollist · 2021-09-30T18:14:20Z

Yee haw! This one is approved!

There are a number tasks that we need to take of now to get this "officially" approved (see #437 (comment)). As this is my first time doing this, we will probably need to learn together on some of these tasks. I'll do my best to keep the process moving along. As you are working on these, let me know if you have any questions or concerns.

I also had a few additions for you to think about:

I notice you are using both Appveyor and GitHub Actions. If Appveyor is still needed, you will need to deal with this task. If not, you can remove the Appveyor CI and rely on the GitHub Actions.
Since you are using codecov, you will likely need to reset the webhook for that.
I also remember that you were planning on submitting your package to CRAN. If that still is the plan, check out the ROpenSci CRAN Gotchas to help along with that submission.

unai-perez · 2021-10-08T07:39:09Z

@hollis, thank you very much for all the advices and comments. Finally, we have completed all the to-dos. Next, we will submit the package to CRAN and if is accepted we will add the badges to the readme. The final changes made on the repository are the following:

To-dos:

 * [x]  Transfer the repo to rOpenSci's "ropensci" GitHub 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]  Fix all links to the GitHub repo to point to the repo under the ropensci organization.

 * [x]  Delete your current code of conduct file if you had one since rOpenSci's default one will apply, see https://devguide.ropensci.org/collaboration.html#coc-file

 * [x]  If you already had a `pkgdown` website **and are ok relying only on [rOpenSci central docs building and branding](https://devguide.ropensci.org/ci.html#even-more-ci-ropensci-docs)**,
   
   * deactivate the automatic deployment you might have set up
   * remove styling tweaks from your pkgdown config but keep that config file
   * replace the whole current `pkgdown` website with a [redirecting page](https://devguide.ropensci.org/redirect.html)
   * replace your package docs URL with `https://docs.ropensci.org/package_name`
   * In addition, in your DESCRIPTION file, include the docs link in the `URL` field alongside the link to the GitHub repository, e.g.: `URL: https://docs.ropensci.org/foobar (website) https://github.com/ropensci/foobar`

 * [x]  Fix any links in badges for CI and coverage to point to the new repository URL.

 * [x]  Please check you have updated the package version to a post-review version and that you documented all changes in NEWS.md

 * [x]  We're starting to roll out software metadata files to all rOpenSci packages via the Codemeta initiative, see https://docs.ropensci.org/codemetar/for how to include it in your package, after installing the package - should be easy as running `codemetar::write_codemeta()` in the root of your package.
 
 * [x]  I notice you are using both Appveyor and GitHub Actions.  If Appveyor is still needed, you will need to deal with this task.  If not, you can remove the Appveyor CI and rely on the GitHub Actions.
 
 * [x]  Since you are using codecov, you will likely need to reset the webhook for that.
 
 * [x]  I also remember that you were planning on submitting your package to CRAN.  If that still is the plan, check out the [ROpenSci CRAN Gotchas](https://devguide.ropensci.org/building.html#crangotchas) to help along with that submission.

jhollist · 2021-10-08T11:31:23Z

@unai-perez Fantastic! Great work on the package.

One last small thing to update is the suggested citation at the bottom of the README. The CRAN citation that you currently have in the CITATION file is fine, but if rsat wont be on CRAN for a while (e.g. more than a month out) go ahead and update this citation to point to the ropensci repo.

Again, nice working with you on this and congratulations!

ropensci-review-bot added the 1/editor-checks label Mar 23, 2021

ropensci-review-bot assigned jhollist May 3, 2021

ropensci-review-bot added 2/seeking-reviewer(s) and removed 1/editor-checks labels May 4, 2021

ropensci-review-bot added 3/reviewer(s)-assigned and removed 2/seeking-reviewer(s) labels Jun 2, 2021

noamross added 4/review(s)-in-awaiting-changes and removed 3/reviewer(s)-assigned labels Jun 29, 2021

noamross added 5/awaiting-reviewer(s)-response and removed 4/review(s)-in-awaiting-changes labels Sep 16, 2021

ropensci-review-bot added 6/approved and removed 5/awaiting-reviewer(s)-response labels Sep 30, 2021

ropensci-review-bot closed this as completed Sep 30, 2021

submission: rsat #437

submission: rsat #437

Comments

unai-perez commented Mar 21, 2021 • edited by ropensci-review-bot Loading

Scope

Technical checks

Publication options

Code of conduct

noamross commented Mar 23, 2021

ropensci-review-bot commented Mar 23, 2021

jhollist commented Mar 23, 2021 • edited Loading

unai-perez commented Mar 24, 2021

jhollist commented Apr 8, 2021 • edited Loading

Editor checks:

Editor comments

Automated Tests

Masked function names

Checks and Tests

Goodpractice

Spelling

Tests

unai-perez commented Apr 9, 2021

jhollist commented Apr 9, 2021 via email

jhollist commented Apr 9, 2021

unai-perez commented Apr 27, 2021

Automated Tests

Masked function names

Checks and Tests

Goodpractice

Spelling

Tests

maelle commented May 3, 2021

ropensci-review-bot commented May 3, 2021

jhollist commented May 3, 2021

unai-perez commented May 4, 2021

jhollist commented May 4, 2021 • edited Loading

jhollist commented May 4, 2021

ropensci-review-bot commented May 4, 2021

jhollist commented May 4, 2021

ropensci-review-bot commented May 4, 2021

jhollist commented May 4, 2021

jhollist commented May 17, 2021

jhollist commented Jun 2, 2021

ropensci-review-bot commented Jun 2, 2021

jhollist commented Jun 2, 2021

mhweber commented Jun 22, 2021

Package Review

Documentation

Functionality

Review Comments

khondula commented Jun 22, 2021

Package Review

Documentation

Functionality

Review Comments

jhollist commented Jun 29, 2021

jhollist commented Jul 13, 2021

unai-perez commented Jul 16, 2021

jhollist commented Jul 16, 2021

jhollist commented Aug 10, 2021

unai-perez commented Sep 9, 2021

jhollist commented Sep 15, 2021

mhweber commented Sep 22, 2021

jhollist commented Sep 29, 2021

jhollist commented Sep 29, 2021

khondula commented Sep 29, 2021

unai-perez commented Sep 30, 2021

jhollist commented Sep 30, 2021

khondula commented Sep 30, 2021

unai-perez commented Sep 30, 2021

khondula commented Sep 30, 2021

jhollist commented Sep 30, 2021

ropensci-review-bot commented Sep 30, 2021

jhollist commented Sep 30, 2021

unai-perez commented Oct 8, 2021

jhollist commented Oct 8, 2021

unai-perez commented Mar 21, 2021 •

edited by ropensci-review-bot

Loading

jhollist commented Mar 23, 2021 •

edited

Loading

jhollist commented Apr 8, 2021 •

edited

Loading

jhollist commented May 4, 2021 •

edited

Loading