-
-
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
Submission: rcites #244
Comments
hi @JonasGeschke Thanks for your submission. I have a clarifying question. What is the relationship of Species+/CITES to IUCN Red List, if any? Does it contain any overlapping information/data? (p.s. you didn't check Do you intend for this package to go on CRAN? - i think you're pkg is already on CRAN, so go ahead and check that) |
Hi @sckott thanks for your quick message. There is no relationship between the IUCN Red List and the CITES Speciesplus database. The IUCN Red List contains data about the conservation/endangerment status of species; the CITES Speciesplus database contains data about the (illegal) trade status of endangered species, so it goes beyond the Red List. Actually, the Speciesplus database also contains data about the protection status of species under the Convention on Migrating Species (CMS), but the API so far only allows access to the CITES legislation status information. Regarding the Do you intend for this package to go on CRAN? question thats right. By mistake, we already submitted the package to CRAN before having it reviewed via ROpenSci. So we dont intend the package to go on CRAN, as it already is on CRAN. Thus, we are looking forward to the reviewing process and how big the first update of the package on CRAN will be. |
Editor checks:
Editor commentsThanks for your submission @JonasGeschke ! Here's the output from goodpractice. If you haven't used ── GP rcites ──────────
It is good practice to
✖ 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/sppplus_simplify.R:29:1
R/sppplus_simplify.R:32:1
R/sppplus_simplify.R:50:1
R/sppplus_taxonconcept.R:35:1
R/sppplus_taxonconcept.R:48:1
... and 30 more lines Seeking reviewers now 🕐 Reviewers: |
@sckott thanks! Actually, following the guidelines of Ropenscience, we've used |
If those lines are already 80 or shorter, then all good. I didn't check them myself. there are definitely exceptions when it's just too cumbersome to do so |
Package Review
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 5 Review CommentsThis is a very useful package to retrieve species CITES status and history. I've actually developed a related package: https://github.com/ecohealthalliance/cites/, which provides access to data pulled from the non-API friendly trade.cites.org. I was very glad to see rcites and it work together just fine! The package is well-constructed and documented. Setting up authentication follows best practices and was fast and easy. The major challenge that the package has to deal with is the complexity of returned objects from the API. I think this could be better handled in both API and documentation. I also think the authors should consider how to deal with workflows for bulk analysis. I detail more on these and a few other things below. Simplifying/processing objectsThe API returns fairly complex records that are challenging to wrangle, which the authors simplify in most functions, with the option of simplifying further with I also note that "special cases" fields that end up as factors when most of them (like URLs) should be characters. I think aggressive simplification should be the default output, and the alternative should be a simple unprocessed list (maybe I note that the functions return Printing objectsBecause the objects returned are complex, and also because they often include very long text fields, they don't print well. There are a couple of ways to approach this. One would be to make the returned objects custom S3 classes (e.g., If you return the raw list I suggest above, you could take a tip from the gh package, give it the class Function APIThe authors use the I note you use None of the functions currently have an argument to define the
Documentation, README, and VignetteThe documentation is very good, and I'm glad that everything refers back to the appropriate API documentation page. I'd suggest just a little more wording in the The README is a bit sparse. It could use more of an introduction, in which the authors describe what the Species+ API is and what data can be retrieved from it, and a few lines of basic usage examples. (See https://ropensci.github.io/dev_guide/building.html#readme). Remember the principle of first entry 1 - any piece of documentation may be the first encounter the user has with the API, data source, or even the CITES treaty/organization. Don't pass up the opportunity to give a curious data scientist some environmental education! The vignette, probably because of the need to make API calls, doesn't actually compile and therefore doesn't show outputs from the code that is run. (The outputs also make for challenging reading, which is why I suggest better printing methods above). This isn't very helpful and I suggest that the authors compile the vignette to a markdown output locally and have a vignette with all the evaluated output blocks (still name While the vignette demonstrates use of a few functions, it would be much more helpful if it explained the type of information that was returned, the structure of the object, and demonstrate a few additional workflow of manipulating these results and using them in analysis, e.g.:
The vigenette doesn't show Bulk AnalysisThe package currently only has non-vectorized functions and focuses on a workflow where one is examining information on a single species. I think there is a lot of potential for enabling different analyses. First, as I mention above, all the taxon concept endpoint parameters should be accessible so that one can retrieve a list of taxa to operate on. A natural extension of this would be to at least show in the vignette how to get the paginated return values, and then iterate over them to retrieve, say, legislation for all those species. I think a better solution would be to build auto-pagination into the taxon concept function, and then add vectorization into the rest of the functions (thinking carefully of how to aggregate all the results into a set of usable data frames). A major change to consider would be to cache/store downloaded data in a local database that can then be queried in more ways. For instance, it would be good to be able to ask "What quotas were started in 2010?". I note that the Species+ API anticipates this, describing a workflow where data is cached in the local client and updated by calling the Tiny stuffNone of the >80 character line widths are egregious or problematic, but I recommend shortening the lines in the examples to this width or less so that they show up help files without overrunning the width of the viewer. Implemented as a PR:
1 https://en.wikipedia.org/wiki/Principle_of_least_astonishment 2 This one I made up a while back, but it's about what we recommend at https://ropensci.github.io/dev_guide/building.html#readme |
Many thanks @noamross for your very careful review of our package. We'll do our best to integrate all of these valuable comments. |
thx for your review @noamross ! |
@noamross thank you for this very comprehensive review. Your comments will help us a lot, we will do our best to address them all! Also I like that your |
@noamross I have two questions regarding your review:
Do you mean JSON/XML? Also, regarding data.table, do you think we should rather not use it? I think we'll be able to do the same manipulations without it, so it means we'll have one less dependency, which may be desired. Thanks! |
Oh! Now I understand! Sorry! |
Correct, I meant the English/French/Spanish parameter. Yes, I'd drop data.table as a dependency. Also, my advice is to hold off on major changes prior to getting your second review. It's usually easier to address them simultaneously, and @mcsiple may not agree with me on everything! |
Hi @JonasGeschke et al. - I am having the same compiling issues as @noamross with the I did a temp fix by adding my own API key to the |
Hi @mcsiple thank you for your post. Yes, making the vignette more comprehensive and with outputs visible is on our list. If you have any further reviewing comments, we are happy to receive those. Thank you! |
The rest of my comments are coming soon - I just wanted to post that one in case other reviewers are assigned and need the compiled vignettes in the meantime! |
Package Review
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 8 General commentsI found this package lovely, easy to use, and well-documented. I think the authors have done a good job of wrangling some complex data types from the API and am sure this package will be widely used. Most of the suggestions I have are about interfacing with the API, as well as a few about usability. Review CommentsDocumentationOnce compiled, the vignette is quite concise and nicely written (There are a few typos in the vignette but none that affect functionality or clarity). Some parts of the vignette could be fleshed out a little more: for example, the "Legislation information" section does not have text to accompany the code. When I compiled the vignette, the The README is also good. As I mentioned in my earlier comment, the API token needs to be set in the environment before the FunctionsSetting the token: There are a lot of Icing to add: improving processing timeOne of the greatest challenges I have with any package that uses an API is the processing time to get bulk entries. For example, I usually have a dataframe for which I need to access and merge data I've gathered through something like Finally, I also think the authors should make tibble output an option for all the users who will be using MappingI love that code for making distribution maps is included in the vignette, but had some issues with the example. The mapping example in the vignette doesn't work- the following error occurs when running I know that this is not the main point of the package, but it's cool that there is an easy way to make these maps with rOpenSci guidelines:
|
Yes!
|
@JonasGeschke thanks for the offer, but i'm just happy to help here |
@KevCaz sometimes I forget, but yes, e.g., you could test that timeout works, e.g., expect_error(some_fxn(httr::timeout(10)), "some message")
# or just that it errors with any message
expect_error(some_fxn(httr::timeout(10))) |
@sckott added and now tested! Thanks for the tip! |
Approved! Thanks again for your submission @JonasGeschke @KevCaz ! To-dos:
We've started putting together a bookdown 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 repo is at https://github.com/ropensci/dev_guide Are you interested in doing a blog post for our blog https://ropensci.org/blog/ ? either a short-form intro to it (https://ropensci.org/technotes/) or long-form post with more narrative about its development (https://ropensci.org/blog/). If so, we'll have our community manager @stefaniebutland get in touch with you on that |
Great, thank you @sckott! We will work on the to-dos asap. |
Great! I can draft a blog post (and let @KevCaz and @JonasGeschke add stuff before doing anything else). @stefaniebutland let me know how to proceed. |
@sckott I didn't get an invitation, so I can't transfer the repo. Can you invite me too? Thanks! |
@ibartomeus sent |
@sckott one more question, what kind of access to the repo do we have? Are we allowed to change the settings of the repo? If yes I cannot do so currently! If no, can you just edit the URL at the home page of the GH repo: https://ibartomeus.github.io/rcites/ => https://ropensci.github.io/rcites/. Thanks! |
@JonasGeschke Great to hear you'd like to contribute a post! This link will give you many examples of blog posts by authors of onboarded packages so you can get an idea of the style and length you prefer. Here are some technical and editorial guidelines for contributing a blog post: https://github.com/ropensci/roweb2#contributing-a-blog-post. We ask that you submit your draft post via pull request a week before the planned publication date so we can give you some feedback. Happy to answer any questions. |
@stefaniebutland thanks for your message, we are very happy to contribute a blog post. @ibartomeus I just put you in the loop so you also see the above two links. |
@sckott just to let you know that |
and with this, all the above points (#244 (comment)) are done. Zenono DOI also is done, as well as a CRAN release 1.0.0. thus I guess we are ready for the JOSS submission, right? do you @sckott have any recommendation for the JOSS editor? |
@JonasGeschke Seems reasonable that I could send a tweet from rOpenSci about the package if that might help? If you think this is a good idea, please send me a short phrase or two that I can use in tweet about pkg along with twitter handle and/or relevant conference hashtag, your twitter handle & that of any pkg co-authors I should tag. |
@JonasGeschke no, nothing else. thanks much.
I'm not familiar with the editors at JOSS - Not sure how long it takes for review at JOSS - but I guess you could just link to the paper in your repo / zenodo? |
@JonasGeschke You can certainly talk about it as "in press" if you want. If you want to accelerate processing, once you submit, you can go to the automatedly generate GitHub issue at https://github.com/openjournals/joss-reviews/issues, and comment that the package has been accepted here, linking back to this issue. |
Thanks for your messages. So I submitted the paper to JOSS. I will let them know about the ropensci review process as told in https://ropensci.github.io/dev_guide/onboarding-guide-for-editors.html#for-joint-joss-submissions
@KevCaz you got this, right? @stefaniebutland thanks, a tweet would be great! I will prepare you a text! Then I guess this issue can be closed ... but this I believe is your task @sckott . Thanks for all your efforts! |
Yep! BTW I was thinking that it may be a good idea to reference this thread or the repo in ropensci-archive/wishlist#29 |
btw: from yesterday to today, the download number jumped from around 620 to almost 1100! 👍 |
So the JOSS paper is published, this is great! @stefaniebutland here its the tweet you can do tomorrow (21.11.2018):
|
Thanks @JonasGeschke. I'll adapt it to fit rOpenSci tweet style and schedule it for 21.11.2018. |
Summary
What does this package do? (explain in 50 words or less):
With rcites we provide an R client to access to the Speciesplus database. We provide functions to 1. access the Speciesplus taxon concept, and thereafter 2. get a species' legislation status, both from CITES and from the European Union, 3. get a species' country-wise distribution range, as listed in Speciesplus, and 4. get the references on which a Speciesplus listing is based.
Paste the full DESCRIPTION file inside a code block below:
URL for the package (the development repository, not a stylized html page):
https://github.com/ibartomeus/rcites
Please indicate which category or categories from our package fit policies this package falls under *and why(? (e.g., data retrieval, reproducibility. If you are unsure, we suggest you make a pre-submission inquiry.):
Database access, because the package gives access to the Species+/CITES Checklist API
Who is the target audience and what are scientific applications of this package?
Researchers and national authorities
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.
Requirements
Confirm each of the following by checking the box. This package:
Publication options
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.Detail
Does
R CMD check
(ordevtools::check()
) succeed? Paste and describe any errors or warnings:Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:
If this is a resubmission following rejection, please explain the change in circumstances:
If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:
https://github.com/karthik
https://github.com/geanders
The text was updated successfully, but these errors were encountered: