Open csaybar opened 1 year ago
@zackarno, thanks a lot for your thorough review!
@csaybar please wait a bit for @nmorandeira's review then you can address both reviews.
Thanks @csaybar for developing this excellent package!, and thanks @maurolepore for the invitation to review it. Sorry for the delay, I'm out of my office with very limited internet access. I provide some comments, mainly related to the documentation. As I said to @maurolepore, my views are as an end-user, since I have no experience in developing a package. I hope you find these comments useful!
The package includes all the following forms of documentation:
[x] A statement of need: clearly stating problems the software is designed to solve and its target audience in README. See below, I suggest adding more detail on this.
[ ] Installation instructions: for the development version of package and any non-standard dependencies in README. See my comment below. Installation instructions are insufficient.
[x] Vignette(s): demonstrating major functionality that runs successfully locally See my comments below.
[ ] Function Documentation: for all exported functions See my comments below.
[x] Examples: (that run successfully locally) for all exported functions See my comments below.
[x] Community guidelines: including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).
I haven't tested functionality, performance, automated tests and packaging guidelines.
Estimated hours spent reviewing: 10; ~3 hours were spent in trying to solve the installation issues.
For example, "While GEE's primary language is JavaScript, which diverges from R's structure, rgeeExtra serves as a bridge". Isn't this true for rgee? What are the additional advantages of using rgeeExtra? I now know the answer :), after using the package and reviewing the Articles, but It would be nice for the user to quickly decide if it's worth installing rgeeExtra or no.
The equation provided as example is confusing because is called “ndvi” but includes a **2. I would name the object _“ndvisquared”, or alternatively remove the ** 2 from the equation.
Add to the Readme.MD in the repository a link to https://r-earthengine.com/rgeeExtra/index.html Articles are a nice vignette! I would highlight them.
Citation: is there an error? In this section, it says
‘citation("rgeeExtra") To cite rgee in publications use:
C Aybar, Q Wu, L Bautista, R Yali and A Barja (2020) rgee: An R package for interacting with Google Earth Engine Journal of Open Source Software URL https://github.com/r-spatial/rgee/.
A BibTeX entry for LaTeX users is
@Article{, title = {rgee: An R package for interacting with Google Earth Engine}, author = {Cesar Aybar and Quisheng Wu and Lesly Bautista and Roy Yali and Antony Barja}, journal = {Journal of Open Source Software}, year = {2020}, } ‘
In R, running citation(“rgeeExtra”) gives the corrected text “To cite **rgeeExtra** in publications use”, but the same reference by Aybar et al. (2020) refering to rgee.
+ **Installation.** This was one of the main issues I encountered while reviewing the package. I was not able to initialize ee, here follows my error (probably related to miniconda or Python?). I couldn’t solve this issue in my laptop. Instead, I decided to conduct the review on other PC were I had already set up my environment. My suggestion is to include a clear step-to-step instruction to install the package and to initialize ee.
I followed the instructions provided here by Mauro: https://github.com/ropensci/software-review/issues/608#issuecomment-1755823040 .
When running `rgee::ee_install() ` I get this error:
![image](https://github.com/ropensci/software-review/assets/6751201/cd46c9e6-8393-49c8-b91c-eb0613589437)
I'm successfully using gee with Python in this laptop, but probably my R + Python environment is not correctly configured.
+ **Function Documentation:** after reviewing the Articles, it's not clear for me if all the functions included in rgeeExtra are listed in the examples. It seems to me that there may be extra functions that I have still not explored. A vignette including the list of functions and its parameters would be appreciated.
## Articles
Here I provide some comments on the articles.
### Introduction
SAVI computation. I get this error:
Error in py_get_attr_impl(x, name, silent) : AttributeError: type object 'Image' has no attribute 'spectralIndex'
Please check this error. I replaced “preprocess” with “Extra_preprocess”, and “spectralIndex” with “Extra_spectralIndex”. Next, I also replaced “geoviz_1” with “geoviz”. In this way I get no error, but I also don’t see any image in the visualizer.
Besides, if it’s possible to compute an spectral index this way, I would add a comment in the Readme file: after giving the ndvi or squared ndvi equation as an example of how an user can define a new formula using the rgeeExtra syntax, I would link to this SAVI example (or an NDVI example) as an additional advantage of the package, i.e. providing an easy way to compute well known spectral indexes.
### Image
Add dependency:
`library(leaflet.extras2)
`
**1. Arithmetic Operations.** The screenshot image shows global coverage, but the code gives a plot with zoom 9. Be consistent so that the user doesn't think he/she has done something wrong.
**2. Mathematical functions.** Comment that you can visualize each NDVI transformation by clicking on the layer selector.
**4. Tasseled Cap Transformation.**
I suggest replacing the display by the most used RGB combination for tasseled cap (R = TCB, G = TCG, B = TCW).
A reference to the citation for the the tasseled cap transformation would be nice, as well as for the other indexes included in the articles (NDVI, SAVI, etc).
**6. Histogram Matching**
The visualization parameters should be different for each image. A possible solution (I have also added the source scene for comparison purposes):
visParams_L8 <- list( bands = c("B4","B3","B2"), min = 0.1, max = 0.5, gamma = 1.5 )
visParams_L7 <- list( bands = c("B3","B2","B1"), min = 0.1, max = 0.5, gamma = 1.5 )
Map$setCenter(lon = -121.984, lat = 47.847, zoom = 14) m0 <- Map$addLayer(source, visParams = visParams_L8, "source") m1 <- Map$addLayer(matched, visParams = visParams_L8, "matched")
m2 <- Map$addLayer(target, visParams = visParams_L7, "target")
m0 | m2
m1 | m2
**7. Cloud masking**
Please check the parameters. I don’t get the same display.
![image](https://github.com/ropensci/software-review/assets/6751201/341e5681-6aaf-4223-a7a4-6eda138fbb74)
**8. Automated Preprocessing**
Please check the parameters. I don’t get the same display.
![image](https://github.com/ropensci/software-review/assets/6751201/557ca1d3-d10f-427b-9b34-f2a3dc43fb3e)
**9. Summary Methods.**
Add visualization instructions.
**10. Minimum and Maximum values.**
Requires geojsonio, which also has several dependencies. Add: `library(geojsonio)`
Also, the text says _“These functions offer two modes: “Rectangle” for using the image’s footprint and “Points” for sampling over it”_. Can you add examples for these two modes?
## Image Collection
**8. Complete GIF Toolkit for Earth Engine.**
Excellent tool! Please add the dependency:
`library(magick)`
Also, save the gif file to the working directory (It was saved in a temporal folder with a random name, so it’s difficult to find it).
**11. Get the temporal nearest image.**
Excellent tool! I would move this example before example (8). Also, the title is in italics, it seems to be an error.
As an user, I would find much more useful to find the closest cloud-free image.
@ropensci-review-bot submit review https://github.com/ropensci/software-review/issues/608#issuecomment-1894601089 time 10
Logged review for nmorandeira (hours: 10)
@ropensci-review-bot submit review https://github.com/ropensci/software-review/issues/608#issuecomment-1879736435 time 16
Logged review for zackarno (hours: 16)
Thanks a lot @nmorandeira for your review! Your perspective is very valuable and really nicely complements that of @zackarno.
@csaybar you can now start addressing the reviews.
Thanks, I will start with the revision.
El mié, 17 ene 2024 a las 2:30, Mauro Lepore @.***>) escribió:
Thanks a lot @nmorandeira https://github.com/nmorandeira for your review! Your perspective is very valuable and really nicely complements that of @zackarno https://github.com/zackarno.
@csaybar https://github.com/csaybar you can now start addressing the reviews.
— Reply to this email directly, view it on GitHub https://github.com/ropensci/software-review/issues/608#issuecomment-1894788609, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD752PRFMT7QJE47CWRMQGTYO4STJAVCNFSM6AAAAAA435BITSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJUG44DQNRQHE . You are receiving this because you were mentioned.Message ID: @.***>
@csaybar, @davemlz, @LBautistaB13, @choisy: 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
Hi @csaybar , I'm checking in on issues that have been static for awhile. Let us know if there is progress to report or any questions you may have.
If the author mentions changes might take time, apply the holding label via typing @ropensci-review-bot put on hold. You’ll get a reminder every 90 days (in the issue) to check in with the package author(s).
--https://devdevguide.netlify.app/softwarereview_editor
--
Hi @csaybar, shall we put this submission on hold?
It's totally fine to do so. The label helps us to better understand and manage the process.
Hi everyone,
I sincerely apologize for my delayed response,
I had a bit of trouble properly solving all of your valid requests. As many of you know, rgee-extra is an extension of rgee. At the same time, rgee relies on the earthengine-api (Python). Therefore, updates to the Python client library directly impact rgee. Recently, the earthengine-api has undergone substantial changes concerning authentication (see updates here and here), which have break many of rgee’s functionalities.
After some internal discussions, we have decided to restructure rgee into a new package (to be named rgee2) to address installation & authentication problems.
@JulioContrerasH is working part-time on developing rgee2, but we do not expect it to be ready before the end of August. We are committed to keeping rgee-extra as an extension of rgee2. However, we prefer to pause further development on the project until rgee2 first version is released.
Thank you for your understanding, and sorry for the lengthy message. :( Yes, @maurolepore, please can u move it to hold.
@csaybar, thanks for your efforts. I'll go ahead and put the submission on hold.
@ropensci-review-bot put on hold
Submission on hold!
Thanks for the update @csaybar Good luck with rgee2!
@maurolepore: Please review the holding status
@csaybar
Is there any change?
The bot is reminding us to review the holding status. This is not to put any pressure but just to ensure we don't forget.
We'll get reminders every three months for a year, then we would need to move forward or close the submission.
Submitting Author Name: Cesar Aybar Submitting Author Github Handle: !--author1-->@csaybar<!--end-author1-- Other Package Authors Github handles: (comma separated, delete if none) @davemlz, @LBautistaB13, @choisy Repository: https://github.com/r-earthengine/rgeeExtra Version submitted: 0.0.1 Submission type: Standard Editor: !--editor-->@maurolepore<!--end-editor-- Reviewers: @nmorandeira, @zackarno
Archive: TBD Version accepted: TBD Language: en
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):
rgeeExtra is an extension of the rgee package that enables interaction with Google Earth Engine (GEE). The advantage of rgeeExtra lies in its user-friendly syntax for R users. The package includes operator overloading and access to various functions created by third parties within the ee_extra package.
GEE users and any R user who wants to retrieve data using the GEE API from within R.
There are other packages for connecting to GEE, but they are primarily focused on composite generation and data download. See geedim
Yes
If you made a pre-submission inquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.
Explain reasons for any
pkgcheck
items which your package is unable to pass.Package uses global assignment operator ('<<-'). The global assignment operator is used in the .onLoad special function to set the ee_extra Python Package in the R global environment (See here). This approach is similar to other reticulate R packages like keras or tensorflow.
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