-
-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chirps: API Client for CHIRPS #357
Comments
Editor checks:
Editor commentsThanks for the submission @kauedesousa. This is the result from
|
Dear @melvidoni thanks for your feedback. I've made the changes and pushed as |
The first reviewer is @cvitolo. I'll fix the review deadline once we have both reviewers. |
We got a second reviewer: @jzwart |
Hello @kauedesousa! This is a very useful package, I work in the field of weather forecasting and I greatly appreciate tools that provide an interface to data services. I found your package easy to use and well documented. All your hard work made my review very easy, thanks and great job! I have some suggestions, which I listed below, I hope you find them useful :) Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing:
Review Comments
|
Dear @cvitolo thank you so much for your comments and suggestions. It helped a lot! We have worked in incorporating the comments to chirps v0.0.6 https://github.com/agrobioinfoservices/chirps. Here we have included a point-to-point response to your comments: general comments
The file README.Rmd was removed
The sf polygon is exported as 'tapajos', the sf POINT object is not necessary and can be generated in the examples with sf. Also, functions dataframe_to_geojson and sf_to_geojson are exported since they had the same issue using ::: in the examples and could be useful for the users
The /man structure is part of using the pre-built vignettes. We not aware of any issues with it. CRAN hasn't baulked at it and Adam (one of the co-authors) just submitted two package updates in the last month that use this method tests folder:
DONE
DONE
We have updated the tests so it checks if the functions return the correct values. For this we downloaded the data from ClimateSERV and compared it with the ones retrieved by get_chirps, get_esi and precip_indices to validate it. The tests still have a skip_on_cran option as a CRAN policy. But we have opened an issue in the package repo and will keep it there until we figure out how to make 'vcr' works with 'chirps' https://github.com/agrobioinfoservices/chirps/issues/7
Same as above
DONE vignettes:
We use this file to speed up the vignette creation, here Jeroen Ooms shows how it works https://ropensci.org/technotes/2019/12/08/precompute-vignettes/
DONE
These warning messages comes from 'sf', but we don't know if is a good practice to suppress that. We added a note to the documentation
Same as above
We added a section for the package limitations. And a better explanation about CHIRPS application into the paper. Also, W. Ashmall says here https://github.com/agrobioinfoservices/chirps/issues/12 that they are planning to upgrade the API service which will make it better to request queries to ClimateSERV. LICENSE = GPL-3
DONE
Thank you, we changed to MIT as suggested inst/paper folder:
DONE
DONE
DONE
DONE
We updated this section with more examples, and hopefully a better explanation on CHIRPS applications and how chirps can help goodpractice::goodpractice():
Same as above in the tests section
The code was removed from seealso in the documentation. Again, thank you for your time reviewing this package. We hope you like its new version. |
Hello @jzwart! Could you submit your review for this package? |
Yes, I'm sorry for the delay but is it possible to get a week extension until Feb. 17 to submit my review? |
Hello @jzwart yes, go ahead. But please, do not delay more than that! |
Hi @melvidoni and @jzwart, yes it is ok for me. |
Hello @jzwart is everything okay? |
Yes I'm currently working on review. |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing:
Review CommentsI installed and reviewed chirps v0.0.6. See the comments above addressing the previous review that went into v0.0.6. The package is well-documented and easy to read. The authors have thoughtfully addressed all previous reviewer comments.
I visited the linked url and tried downloading some precip data using a custom polygon and there was no response for the ~20 minutes I waited for data to return. I'm not sure if this is a common issue with this server, but I don't feel like I can finish the review until the server is working and returns data.
It's pretty clear that the server is down, but if a user does not know that the server is down, would you expect multiple As long as the server is up, the package seems to work well. I just have a few comments to help the user in debugging instances above ^. |
The data call request seems to be working this morning, I'll finish my review and update my comment above |
Hi @jzwart I was informed by the developers of ClimateSERV that they are doing some improvement in the server. I just tried using both Otherwise what should we do @melvidoni ? |
Thanks @kauedesousa , |
@kauedesousa and @melvidoni, I've updated my review above. Although I couldn't finish all instances of the tests, I'm confident that they'd work if the server was working. I just have some comments on improvements for debugging and description of best uses / limitations for the package. |
Hi @melvidoni I am a bit busy at work and I think I will not be able to send a revised version of |
Hello @kauedesousa. That is perfectly fine, take your time. Please advise if you are going to need more time, so I can apply the correct labels. |
In the interest of reducing load on reviewers and editors as we manage the COVID-19 crisis, rOpenSci is temporarily pausing new submissions for software peer review for 30 days (and possibly longer). Please check back here again after 17 April for updates. In this period new submissions will not be handled, nor new reviewers assigned. Reviews and responses to reviews will be handled on a 'best effort' basis, but no follow-up reminders will be sent. Other rOpenSci community activities continue. We express our continued great appreciation for the work of our authors and reviewers. Stay healthy and take care of one other. The rOpenSci Editorial Board |
In this period new submissions will not be handled, nor new reviewers assigned.
Reviews and responses to reviews will be handled on a 'best effort' basis, but
no follow-up reminders will be sent. Please check back here again after 25 May when we will be announcing plans to slowly start back up. We express our continued great appreciation for the work of our authors and reviewers. Stay healthy and take care of one other. The rOpenSci Editorial Board |
Dear all, I hope you are doing fine during this crisis. We managed to address the comments from our reviewers and hereby we submit the revised version of chirps in its version 0.0.8. All the best general commentsCould you clarify what 'near-present' means in the Overview section of the README.md?
Can you add more details for data return performance and what the user should expect for big data calls in the README? Is the package most useful for a few point data returns or can the user get a continental data call returned in a reasonable amount of time? I see some description of limitations in the Overview vignette, but I think the best uses for the chirps dataset, package, and any limitations should be upfront so a potential user can decide to install the package before finding a limitation description in the vignette. I could not run the example code successfully. I let the example code in the README run for about 20 min before it timed out and returned an error.
Is there a recommended timeout for the data request? Although get_chirps() worked earlier this morning, it seems like the server is down again, and I get the following output with the paper's example script:
It's pretty clear that the server is down, but if a user does not know that the server is down, would you expect multiple 0%... data progress from big data calls? Would this data call timeout if I left it going when the server is down? A little more guidance on the call request time would help the user debug in these instances.
|
Hello @kauedesousa, thanks for the updates! @cvitolo @jzwart I hope you are safe. Can you please review the changes made by the author? |
Hi @melvidoni and @kauedesousa, |
Hi @melvidoni, I’m very happy with the revised version. All my suggestions have been addressed. @kauedesousa great job! |
Nice job on this package, @melvidoni, and thank you for the careful consideration of all the reviews. I find the revised package very easy to use and the descriptions in the vignettes & README useful. Thank you for adding in the time out for |
Approved! Thanks @kauedesousa for submitting and @cvitolo @jzwart for your reviews! To-dos:
Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them 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've put together 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, the corresponding repo is here. |
Thanks @melvidoni ! I will follow the to do list. Thanks @cvitolo and @jzwart for the helpful comments and suggestions. Would you like to be acknowledged in the pkg Description with the "rev" role? |
Great! Please, @kauedesousa tag me if you need any assistance. |
I already need. Can you assign the repo to the team chirps that you set up? I skipped this step and now I can't go into the repo settings. |
Hello there! I was just trying to add the team, please be patient. |
@kauedesousa, no need to acknowledge me as a reviewer in the package description, but thank you for asking! |
@kauedesousa, congratulations on getting your package approved! No need to acknowledge me as reviewer either but I would double check with @melvidoni if rOpenSci advises otherwise. |
Hi @melvidoni , the to-do list is completed. Let me know if there is something else to do. My next step is submit this to CRAN. |
Good luck! |
Submitting Author: Kauê de Sousa (@kauedesousa)
Repository: https://github.com/agrobioinfoservices/chirps
Version submitted: 0.0.4
Editor: @melvidoni
Reviewer 1: @cvitolo
Reviewer 2: @jzwart
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):
chirps automates downloading and parsing CHIRPS data into a useful format in R using the ClimateSERV API https://climateserv.servirglobal.net/.
Who is the target audience and what are scientific applications of this package?
Researchers who use CHIRPS data for modelling or other purpose.
Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?
No.
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
JOSS Options
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.MEE Options
Code of conduct
The text was updated successfully, but these errors were encountered: