Closed unai-perez closed 2 years ago
@ropensci-review-bot assign @jhollist as editor
Assigned! @jhollist is now the editor
@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.
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.
@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.
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!
*
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. 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.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))
Many issues are raised with goodpractice::gp()
. Pay close attention to the output and fix these issues. In particular:
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.
[x] *
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.
[x] *
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
styler
package will help with some of these, but not all (e.g. 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. ✖ 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
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
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. *
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.@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.
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 https://github.com/ropensci/software-review/issues/437#issuecomment-816824115, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJPYSYZCZK7SVJO4ILYSRLTH4XWRANCNFSM4ZSADESA .
-- Jeffrey W. Hollister email: @.*** cell: 401 556 4087 https://jwhollister.com
@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.
@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.
* [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.
* [ ] 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.
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))
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
* [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
* [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.
@ropensci-review-bot assign @jhollist as editor
Assigned! @jhollist is now the editor
@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:
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.Any questions or concerns about this, just let me know.
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.
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.
@ropensci-review-bot seeking reviewers
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
@ropensci-review-bot add @khondula to reviewers
@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.
@khondula As we discussed via email, if you need additional time on the review, just let me know.
@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.
@ropensci-review-bot add @mhweber to reviewers
@mhweber added to the reviewers list. Review due date is 2021-06-23. Thanks @mhweber for accepting to review!
@ropensci-review-bot reviewers assigned
@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.
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
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).Estimated hours spent reviewing: 10
A useful and well-documented package
Please add contributing guidelines to README
Tests currently have 1 Fail and 64 Warnings:
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?
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.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.
Hi @jhollist and @unai-perez , please find my review below.
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
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).Estimated hours spent reviewing:
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"’
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.
@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.
@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.
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:
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.
@unai-perez Sounds like a good plan. I'll check on your progress in a few weeks.
@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.
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.
@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!
@jhollist , @unai-perez my comments and suggestions have all been addressed - appreciate the work in making these updates!
Thanks, @mhweber!
@khondula Do these edits address the suggestions in your review?
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
@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.
I can confirm that with install_github("spatialstatisticsupna/rsat", build_vignettes = TRUE)
I see the 4 vignettes.
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!
@khondula, you're right! I fixed that bug. Now, the single images are displayed correctly with plot.
@jhollist, @unai-perez has addressed everything from my review
@ropensci-review-bot approve rsat
Approved! Thanks @unai-perez for submitting and @khondula, @mhweber for your reviews! :grin:
To-dos:
pkgdown
website and are ok relying only on rOpenSci central docs building and branding,
pkgdown
website with a redirecting pagehttps://docs.ropensci.org/package_name
URL
field alongside the link to the GitHub repository, e.g.: URL: https://docs.ropensci.org/foobar (website) https://github.com/ropensci/foobar
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.
Yee haw! This one is approved!
There are a number tasks that we need to take of now to get this "officially" approved (see https://github.com/ropensci/software-review/issues/437#issuecomment-931550150). 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:
@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.
@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!
Date accepted: 2021-09-30 Submitting Author: Unai Pérez-Goya (@unai-perez)
Due date for @khondula: 2021-05-25 Due date for @mhweber: 2021-06-23Other 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: !--editor-->@jhollist<!--end-editor-- Reviewers: @khondula, @mhweber
Archive: TBD Version accepted: TBD
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.):
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.
This package:
Publication options
[x] 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](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) 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