search

ropensci / software-review

rOpenSci Software Peer Review.
286 stars 104 forks source link

Implementation of OPTRAM algorithm to derive soil moisture from remote sensing imagery #612

Closed micha-silver closed 3 weeks ago

micha-silver commented 8 months ago

Date accepted: 2024-06-05

Submitting Author Name: Micha Silver Submitting Author Github Handle: !--author1-->@micha-silver<!--end-author1-- Other Package Authors Github handles: (comma separated, delete if none) @github_handle1, @github_handle2 Repository: https://gitlab.com/rsl-bidr/roptram Version submitted: 0.0.1.000 Submission type: Standard Editor: !--editor-->@adamhsparks<!--end-editor-- Reviewers: @harryeslick, @obrl-soil

Archive: TBD Version accepted: TBD Language: en

Package: rOPTRAM
Title: Derive soil moisture using the OPTRAM algorithm
Version: 0.0.1.000
Authors@R: c(
  person("Micha", "Silver", , "silverm@post.bgu.ac.il", role = c("aut", "cre"),
           comment = c(ORCID = "0000-0002-1128-1325")),
  person("Arnon", "Karnieli", , "karnieli@bgu.ac.il", role = c("ctb", "fnd"),
          comment = c(ORCID = "0000-0001-8065-9793"))
  )
Description: The OPtical TRapezoid Model (OPTRAM) derives soil moisture
  based on the linear relation between a vegetation index
  and Land Surface Temperature (LST). The Short Wave Infra-red (SWIR) band
  is used as a proxy for LST. See:
  Sadeghi, M., Babaeian, E., Tuller, M., Jones, S.B., 2017.
  The optical trapezoid model:
  A novel approach to remote sensing of soil moisture 
  applied to Sentinel-2 and Landsat-8 observations.
  Remote Sensing of Environment 198, 52–68,
  https://doi.org/10.1016/j.rse.2017.05.041 .
License: GPL (>= 3) + file LICENSE
URL: https://gitlab.com/rsl-bidr/roptram.git
BugReports: https://gitlab.com/rsl-bidr/
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.2.3
Depends:
  R (>= 4.1.0)
Imports:
    dplyr,
    ggplot2,
    sf,
    terra,
    tools,
    utils
Suggests:
    geojsonio,
    geojsonlint,
    stats,
    sen2r (> 1.5.0),
    testthat (>= 3.0.0),
    xml2
Config/testthat/edition: 3

Scope

Technical checks

Confirm each of the following by checking the box.

This package:

Publication options

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

adamhsparks commented 6 months ago

@obrl-soil, as already agreed, please take some extra time with your field work and the holidays coming up. I’ll follow up mid-late January if I don’t hear back from you by then.

harryeslick commented 6 months ago

Hi @micha-silver , I'm looking forward to working with you on this review. As mentioned by adamhsparks I am on leave until mid-Jan so won't have much time to meaningfully contribute before then.

I've just been having a superficial look and thought I would share some thoughts:

This package uses the R package for acquiring Sentinel-2 imagery. To install that package...

Could be made more clear using curly bracket convention to show R packages eg.

{rOPTRAM} uses {sen2r} for acquiring Sentinel-2 imagery. To install {sen2r}...

micha-silver commented 6 months ago

Hi @harryeslick :

Thanks for the early comments. I'll have a look at replacing the references to external packages, as you suggested.

Regarding {sen2r}, indeed the maintainer has gone to other things, and what's worse, part of his earlier implementation (accessing the Copernicus API) no longer works since the API has been changed. We are now actively working on alternatives. This will probably include the new {CDSE} package, now on CRAN, that offers access thru the new Copernicus API. I'd like to go ahead with the review process looking at the package as is, currently. If it's accepted to ROpenSci, then we'll move on to a second version once we have implemented and tested the image acquisition changes.

Looking forward to further responses ... next year. Happy holidays.

Regards, Micha

ropensci-review-bot commented 6 months ago

:calendar: @harryeslick you have 2 days left before the due date for your review (2023-12-29).

ropensci-review-bot commented 6 months ago

:calendar: @obrl-soil you have 2 days left before the due date for your review (2023-12-29).

adamhsparks commented 6 months ago

I think that it's necessary to remove any dependencies on a package that is no longer supported, i.e., {sen2r}. Please make the necessary changes to ensure that {rOPTRAM} will work as anticipated by users before we accept it into rOpenSci so that the reviewers may review the package as it will be used. We can place a hold on the reviews if necessary for these changes to be made.

Just by way of information, I had a situation with {nasapower} where the NASA team actually implemented a proper API while I had the original version of the package in-review, we waited to finalise the review until I basically rewrote the entire package due to the changes with the data source. This is much more minor than that was.

micha-silver commented 6 months ago

OK, so I understand the review is on "freeze" until I remove the deprecated {sen2r}, and implement the new API. We're now working on the replacements for {sen2r}, so the complete package should be ready "soon...".

adamhsparks commented 6 months ago

@ropensci-review-bot put on hold

ropensci-review-bot commented 6 months ago

Submission on hold!

adamhsparks commented 6 months ago

@harryeslick and @obrl-soil, I've placed a "Holding" label on this review; please hold off on your reviews until the appropriate changes have been made to the package. If you are unable to complete your reviews at a later date just ping me and I'll get it sorted.

micha-silver commented 3 months ago

Hello @adamhsparks I'm happy to inform that we've completed removal of the deprecated {sen2r} package, and replaced it with the new {CDSE} package for access to Sentinel imagery. We also added additional vignettes, as well as two new curve fitting options to create the OPTRAM 'trapezoid'. Today I ran the CI pipeline, and all tests passed on the gitlab installation (ubuntu release). Coverage is at 87.9%. I also request tests, from the CI pipeline using rhub on Windows and Mac, and these are failing since a few tests require login credentials. (I don't yet know how to send the creds to rhub. I'll check how to use environment variables...)

On Ubuntu release:

── R CMD check results ────────────────────────────────── rOPTRAM 0.1.0.000 
Duration: 1m 35.8s
❯ checking installed package size ... NOTE
    installed size is  8.1Mb
    sub-directories of 1Mb or more:
      doc       2.2Mb
      extdata   5.6Mb
0 errors ✔ | 0 warnings ✔ | 1 note ✖
rOPTRAM Coverage: 87.96%

Other platforms:

ℹ Check
  <https://mac.r-project.org/macbuilder/results/1709624544-1d521aff36a481d0/>
  the results in 5-10 mins (~07:52 AM).
               platform errors warnings notes
 windows-x86_64-release      1        0     1
  windows-x86_64-oldrel      0        0     0
   windows-x86_64-devel      1        0     1
          macos_release      6        4    18
[1] "2024-03-05 07:52:26.288169  - Check completed"
[1] "Elapsed time for checking:  24.1 minutes"
Error: Some checks with ERROR, or WARNING.
Execution halted

Please consider releasing the hold, and approaching reviewers to begin their review of the package. Thanks,

adamhsparks commented 3 months ago

@ropensci-review-bot remove hold

ropensci-review-bot commented 3 months ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@ropensci-review-bot help

adamhsparks commented 3 months ago

@ropensci-review-bot help

ropensci-review-bot commented 3 months ago

Hello @adamhsparks, here are the things you can ask me to do:


# Add a review's info to the ROpenSci logs
@ropensci-review-bot submit review <REVIEW_URL> time <REVIEW_HOURS(ex. 10.5)>

# List all available commands
@ropensci-review-bot help

# Show our Code of Conduct
@ropensci-review-bot code of conduct

# Checks peer-review badge is in README.md
@ropensci-review-bot check readme

# Switch to 'seeking reviewers'
@ropensci-review-bot seeking reviewers

# Approves a package. This command will close the issue.
@ropensci-review-bot approve package-name

# Invite the author of a package to the corresponding rOpenSci team. This command should be issued by the author of the package.
@ropensci-review-bot invite me to ropensci/package-name

# Adds package's repo to the rOpenSci team. This command should be issued after approval and transfer of the package.
@ropensci-review-bot finalize transfer of package-name

# Mint package as [bronze/silver/gold]
@ropensci-review-bot mint silver

# Add a user to this issue's reviewers list
@ropensci-review-bot assign xxxxx as reviewer

# Remove a user from the reviewers list
@ropensci-review-bot remove xxxxx from reviewers

# Assign a user as the editor of this submission
@ropensci-review-bot assign @username as editor

# Put the submission on hold for the next 90 days
@ropensci-review-bot put on hold

# Remove the editor assigned to this submission
@ropensci-review-bot remove editor

# Change or add a review's due date for a reviewer
@ropensci-review-bot set due date for @reviewer to YYYY-MM-DD

# Close the issue
@ropensci-review-bot out of scope

# Various package checks
@ropensci-review-bot check package

# Checks srr documentation for stats packages
@ropensci-review-bot check srr
micha-silver commented 3 months ago

Update: I added a skip_if_not to avoid the test for the credentials when the creds file is not available. So now the Windows builds go thru cleanly. But MacOS still not :-(

adamhsparks commented 3 months ago

@harryeslick and @obrl-soil, sorry for the delay. I'll set the due date to three weeks from now. If you need more time, feel free to take it, please just let us know you need more time.

adamhsparks commented 3 months ago

@ropensci-review-bot set due date for @harryeslick to 2024-03-25

ropensci-review-bot commented 3 months ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@ropensci-review-bot help

adamhsparks commented 3 months ago

@ropensci-review-bot set due date for @harryeslick to 2024-03-25

ropensci-review-bot commented 3 months ago

Review due date for @harryeslick is now 25-March-2024

adamhsparks commented 3 months ago

@ropensci-review-bot set due date for @obrl-soil to 2024-03-25

ropensci-review-bot commented 3 months ago

Review due date for @obrl-soil is now 25-March-2024

micha-silver commented 3 months ago

Hi @adamhsparks Just a quick update: we've fixed the final two (minor) issues with the MacOS build. Now I'm getting no errors and no warnings on all 5 platforms. And coverage is at 92% :-)

micha-silver commented 3 months ago

Hi @adamhsparks No response yet from either of the reviewers. Is it time for a gentle reminder?

obrl-soil commented 3 months ago

Hey - sorry but the rewrite pushed this from a quiet time for me to a very busy one! I can get to this over the long weekend.

harryeslick commented 3 months ago

Hi @micha-silver , Im working my way through. I've logged an issue for you in GitLab: rsl-bidr/roptram#1

micha-silver commented 3 months ago

@harryeslick Thanks for finding time for the review.
This issue is fixed in the gitlab repo: 99c04bff. I improved the check_swir_band function, and also changed the example.

harryeslick commented 3 months ago

@micha-silver , here is my review, For the most part it all works quite easily, so well done! my comments as all minor things.

I still need to read up on the Packaging guidelines so Ive not quite completed it yet, so some jobs for me to follow up on as well.

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

Documentation

The package includes all the following forms of documentation:

some minor suggestions:

OAuth link missing

Running the main wrapper function optram() in the example states that

Registration on Copernicus DataSpace was done in advance, and the OAuth credentials (clientid, and secret) have been saved to the user’s home directory.

I would i like to have a link here to the vignette where setting up OAuth is demonstrated.

OAuth example suggestion

This is a bit of a stylistic preference, and keep in mind I don't fully understand the workflow, so I could be totally wrong here... roptram/docs/articles/rOPTRAM_acquiring.html#step-4-saving-credentials

Saving credentials section seems a little long winded and unnecessary. I think it could probably be replaced with just 

store_cdse_credentials("my_clientID", "My_secret")

the convenience of having a save_creds = TRUE, seems to require more time to explain that it saves in the actual workflow. but I could be wrong.

Is this meant to be optram_calculate_soil_moisture()?

ALSO: optram_calculate_soil_moisture fails with default trapezoid_method trapezoid_method is called (Line92) before arg.match(trapezoid_method) (Line108)

I found a few minor bugs here:

acquire_scihub(aoi, from_date, to_date, veg_index = "SAVI") returns error

SWIR band must be either 11 or 12

optram_acquire_s2

  1. comma missing after parameter remote = "scihub"
  2. SWIR_Band default fails:

optram_calculate_soil_moisture

  1. VI_dir "SAVI" does not exist. Should this be "NDVI"?
  2. data_dir system.file call missing package parameter. should be data_dir <- system.file("extdata", package = "rOPTRAM")

optram_calculate_str returns NULL data_dir system.file call missing package parameter. should be: BOA_dir <- system.file("extdata", "BOA", package = "rOPTRAM")

optram_ndvi_str returns NULL VI_list system.file call missing package parameter. Also using SAVI instead of NDVI STR_list system.file call missing package parameter.

optram_wetdry_coefficients Please check example here. not sure if the plot works...

plot_vi_str_cloud Please check example here. not sure if the plot works...

retrieve_cdse_credentials No example provided, but not sure if one is required. Does this function need to be exported?

store_cdse_credentials No example provided

Functionality

Estimated hours spent reviewing: 6

Other related issues logged at GitLab

rOPTRAM::optram default argument SWIR_band is invalid

issue related to default parameter values logged as issue here: https://gitlab.com/rsl-bidr/roptram/-/issues/1

CDSE credentials saved and retrieved from different file paths

credentials bug logged : https://gitlab.com/rsl-bidr/roptram/-/issues/2

micha-silver commented 2 months ago

@harryeslick With the latest commit we have (I think) addressed all the suggestions that you raised. Any further comments are welcome! (How can I get your email addr to add you as 'rev' to the DESCRIPTION file?)

obrl-soil commented 2 months ago

Hey, sorry this took so long! Not sure I caught everything but I think I've covered off on the main issues. Tl;dr, good job overall but I think the function design is trying to do a little too much for the end user in places.

Package Review

Documentation

The package includes all the following forms of documentation:

Functionality

Estimated hours spent reviewing: 8

Review Comments

General

Vignettes

Documentation

see https://roxygen2.r-lib.org/articles/formatting.html .

For specific functions,

aquire_scihub()

exponential_coefficients()

optram_landsat()

optram_wetdry_coefficients()

Examples

aquire_openeo()

aquire_scihub()

acq_sh <- acquire_scihub(aoi, from_date, to_date,
            veg_index = "SAVI",
            clientid = 'paste id here',
            secret = 'paste secret here',
            SWIR_band = 11)

to prompt the user directly.

calculate_vi()

crop_landsat_list()

exponential_soil_moisture(), linear_soil_moisture(), optram_calculate_soil_moisture()

optram_acquire_s2()

optram_calculate_str()

optram_ndvi_str()

optram_prepare_other_vi_str()

polynomial_coefficients()

store_cdse_credentials()

Tests

micha-silver commented 2 months ago

Hi @obrl-soil Thanks for the detailed review! We'll start going over, point by point, and let you know when everything has been addressed. (Can you send me your email, so that I can add you as 'rev' in the DESCRIPTION file?)

micha-silver commented 2 months ago

@adamhsparks , @obrl-soil , @harryeslick : In @obrl-soil 's review, she raises an interesting point:

"I don't think your functions should be reading spatial data in for the users, and I'm specifically focusing on the aoi_file parameter here. It creates a lot of room for weird errors which you'll then be pestered about. Instead, I think it would be far safer to require the user to read in a file using sf and supply the resultant object to aoi_file. This then gives you the ability to write unit tests and error catches to cover e.g. correct spatial type supplied, data projected in an acceptable CRS, non-insane extents, etc. You can't do any of that when all you have to work with is a file path string."

I chose to allow users to enter a file name for the area of interest, instead of requiring them to read their spatial file as an sf object, and passing that thru to the various functions. This choice was for two reasons: I use the file name (without file extension) later to add a title to the derived scatter plot. And second, I do not assume users would necessarily have knowledge of the {sf} package. However @obrl-soil suggests not to save the scatterplot to an image file at all, rather let the user do that by herself. So the first reason above would be canceled, leaving only the second reason: user's handling of {sf} objects by herself.

I'd like to ask your opinions if it's reasonable to expect user's to understand the {sf} package well enough to read in their aoi_file, and pass an sf object to the OPTRAM functions. (This would certainly save some complexity in the rOPTRAM package)

Thanks,

adamhsparks commented 2 months ago

I agree with @obrl-soil on this. Too many edge cases. Users can be expected to have a modest understanding of {sf} to use a package that leverages it. Simplify your package. Future you will thank you.

In the past I added “support” for operations outside the focus of my package, in time I simplified following the UNIX philosophy. Do one thing. Do it well. No one ever complained about the “loss” of functionality as it was already in R with other packages.

adamhsparks commented 2 months ago

I forgot to do this.

adamhsparks commented 2 months ago

@ropensci-review-bot submit review https://github.com/ropensci/software-review/issues/612#issuecomment-2027336215 time 6

ropensci-review-bot commented 2 months ago

Logged review for harryeslick (hours: 6)

adamhsparks commented 2 months ago

@ropensci-review-bot submit review https://github.com/ropensci/software-review/issues/612#issuecomment-2053591926 time 8

ropensci-review-bot commented 2 months ago

Logged review for obrl-soil (hours: 8)

micha-silver commented 2 months ago

Hello all: First a big thanks to the reviewers for their insightful comments. :clap: :clap: We have gone over the list and modified as follows:

Looking forward to any additional suggestions.

micha-silver commented 2 months ago

Also... I'm looking for advice on one issue: We implement two functions for building the model from pre-downloaded Landsat, or Sentinel imagery. Users who choose to download manually can build the module using optram_safe() on a directory of *SAFE folders, and optram_landat() with a directory of Landsat folders. I do not see any way to show examples, or write tests since both of these require many GB of imagery as input - way too much for a github repo. How should I present this? Currently, in the examples, I sidestep the problem and put:

#' \dontrun{
#' aoi <- sf::st_read(system.file("extdata",
#'                               "lachish.gpkg", package = "rOPTRAM"))
#' # landsat_dir is a directory containing the original downloaded Landsat images.
#' landsat_dir <- "...enter full path here..."
#' optram_landsat(landsat_dir,  aoi,
#'                veg_index = 'SAVI',
#'                LC_output_dir = tempdir(), data_output_dir = tempdir())
#' }
adamhsparks commented 2 months ago

@ropensci-review-bot submit response https://github.com/ropensci/software-review/issues/612#issuecomment-2084811325

ropensci-review-bot commented 2 months ago

I'm sorry @adamhsparks, I'm afraid I can't do that. That's something only author1 and author-others are allowed to do.

adamhsparks commented 2 months ago

Thanks, @micha-silver, @obrl-soil and @harryeslick, can you please check if you are satisfied with the changes and if you have additional comments?

adamhsparks commented 2 months ago

Also... I'm looking for advice on one issue: We implement two functions for building the model from pre-downloaded Landsat, or Sentinel imagery. Users who choose to download manually can build the module using optram_safe() on a directory of *SAFE folders, and optram_landat() with a directory of Landsat folders. I do not see any way to show examples, or write tests since both of these require many GB of imagery as input - way too much for a github repo. How should I present this? Currently, in the examples, I sidestep the problem and put:

#' \dontrun{
#' aoi <- sf::st_read(system.file("extdata",
#'                               "lachish.gpkg", package = "rOPTRAM"))
#' # landsat_dir is a directory containing the original downloaded Landsat images.
#' landsat_dir <- "...enter full path here..."
#' optram_landsat(landsat_dir,  aoi,
#'                veg_index = 'SAVI',
#'                LC_output_dir = tempdir(), data_output_dir = tempdir())
#' }

This is how I would handle the example.

Do the data requirements expand beyond the need to download the data? That is, once the data is downloaded, is the pathway for the model the same and is therefore tested with the normal tests?

micha-silver commented 2 months ago

This is how I would handle the example.

Do the data requirements expand beyond the need to download the data? That is, once the data is downloaded, is the pathway for the model the same and is therefore tested with the normal tests?

Yes, that's exactly what happens.

adamhsparks commented 2 months ago

I'd not worry about tests for this then.

ropensci-review-bot commented 1 month ago

@micha-silver, @github_handle1, @github_handle2: please post your response with @ropensci-review-bot submit response <url to issue comment> if you haven't done so already (this is an automatic reminder).

Here's the author guide for response. https://devguide.ropensci.org/authors-guide.html

micha-silver commented 1 month ago

@ropensci-review-bot submit response https://github.com/ropensci/software-review/issues/612#issuecomment-2084811325

ropensci-review-bot commented 1 month ago

Logged author response!