ropensci / software-review

rOpenSci Software Peer Review.
286 stars 104 forks source link

submission: rppo #207

Closed jdeck88 closed 6 years ago

jdeck88 commented 6 years ago

Summary

Package: rppo
Title: R functions to access Plant Phenology Ontology annotated datasets
Date: 2018-03-21
Version: 1.0
Authors@R: as.person(c(
    "John Deck <jdeck88@gmail.com> [aut, cre]", 
    "Brian Stucky <stuckyb@flmnh.ufl.edu> [aut]",
    "Ramona Walls <rwalls@cyverse.org> [aut]",
    "Kjell Bolmgren <Kjell.Bolmgren@slu.se> [aut]",
    "Ellen Denny <ellen@usanpn.org> [aut]",
    "Robert Guralnick <rguralnick@flmnh.ufl.edu> [aut]"
  ))
Maintainer: John Deck <jdeck88@gmail.com>
Description: Access data from the Plant Phenology Ontology data store.
Depends: R (>= 3.4.2)
License: CC0
Encoding: UTF-8
LazyData: true
RoxygenNote: 6.0.1.9000
Imports:
    rjson,
    readr,
    plyr,
    leafletR (>= 0.1-1),
    httr
Suggests: testthat,
    knitr,
    rmarkdown
VignetteBuilder: knitr

https://github.com/biocodellc/rppo/

This package fits under data retrieval since it is used in downloading/extracting data from the global plant phenology data portal.

The target audience are scientists researching plant phenology patterns, shifts of plant phenological patterns over time and space, climate change biologists, and biodiversity scientists looking for sources of plant observation data.

There are no other R packages that we know of that do the same thing.

We enquired with @sckott and @karthik several weeks ago.

Requirements

Confirm each of the following by checking the box. This package:

Publication options

Detail

annakrystalli commented 6 years ago

Editor checks:


Editor comments

Hello @jdeck88, sorry for the slight delay and many thanks for your submission! An interesting package.

So there's a few issues flagged by the initial editol checks:

testthat.R missing

The package repository is missing a testthat.R script in the tests/ directory. Because of this, running covr::package_coverage() returns 0% coverage. Using usethis::use_testthat() creates the required file and the tests run as expected.

goodpractice::gp output

── GP rppo ──────────────────────────────────────────────────────────────────────────────────────
  It is good practice to

  ✖ write unit tests for all functions, and all package code in general. 75%
    of code lines are covered by test cases.

    R/ppo_data.R:74:NA
    R/ppo_data.R:75:NA
    R/ppo_data.R:76:NA
    R/ppo_data.R:77:NA
    R/ppo_data.R:78:NA
    ... and 23 more lines
  ✖ omit "Date" in DESCRIPTION. It is not required and it gets invalid quite
    often. A build date will be added to the package when you perform `R CMD build` on
    it.
  ✖ 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] Change `'='` assignement 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/ppo_data.R:33:1
R/ppo_data.R:39:1
R/ppo_data.R:50:1
R/ppo_data.R:58:1
R/ppo_data.R:72:1
... and 5 more lines
- [x] check code line length

✖ omit trailing semicolons from code lines. They are not needed and most R coding standards forbid them

R/ppo_data.R:41:64
R/ppo_data.R:119:32
R/ppo_data.R:120:17
R/ppo_terms.R:35:59
- [x] omit trailing semicolons 

✖ not import packages as a whole, as this can cause name clashes between the imported packages. Instead, import only the specific functions you need.

- [ ] **reviewers** to consider and give feedback on

✖ fix this R CMD check NOTE: Namespace in Imports field not imported from: ‘leafletR’ All declared Imports should be used.

- [ ] Needs checking. Usually this means you have a package (`leafletR`) in `Imports` that should really be in `Suggests` (perhaps it is only used in a vignette).

─────────────────────────────────────────────────────────────────────────────────────────────────


#### `devtools::spell_check()`

Flags some potential spelling errors of which only this looks genuine:

pouplates      ppo_terms.Rd:20

CC0 License.

Although the CC0 license specified in the DESCRIPTION is accepted by CRAN, it is not accepted by OSI. We recommend avoiding it unless you have a specific need to use it and suggest considering MIT as a similarly permissible license.


Happy to start looking for reviewers. I recommend using this time to try and address some of the issues raised so they can be checked during review rather that be reflagged.

Let me know if anything is unclear!


Reviewers: Due date:

jdeck88 commented 6 years ago

Thank you for the comments Anna.. I have addressed all of the issues that you brought up and committed my changes to our Github repo.

annakrystalli commented 6 years ago

Thanks for your prompt response @jdeck88! I've found the first reviewer and just trying to secure the second one. Will keep you posted. 👍

annakrystalli commented 6 years ago

Both reviewers have now been assigned! 🎉


Reviewers: @tdjames1 & @remsamp Due date: 2018-05-04

annakrystalli commented 6 years ago

👋 @tdjames1 & @remsamp. Just checking in to make sure you're review is going ok? Feel free to reach out if have any questions 😊

tdjames1 commented 6 years ago

@annakrystalli I'm just getting onto the review now, apologies for the delay. I'm aiming to get it done over this weekend.

annakrystalli commented 6 years ago

No problem @tdjames1, thanks for letting me know. I'll push back due date to the 7th then.


Reviewers: @tdjames1 & @remsamp Updated due date: 2018-05-07

remsamp commented 6 years ago

Hi @annakrystalli and @jdeck88, here's my review (apologies for the slight delay).

Package Review

Documentation

The package includes all the following forms of documentation:

Functionality

Estimated hours spent reviewing: 4


Review Comments

The Rppo package allows programmatic access to the global Plant Phenology (PPO) data portal. This is a compact but well put together package that will come useful to many plant ecologists and evolutionary biologists. Note that although both ppo_data() and ppo_terms() have mostly worked smoothly, I have also experienced some server-related issues while running them. I have provided details below as well as a few other comments/suggestions.

Documentation

I have had difficulties with accessing the rppo vignette from R. This is the relevant devtools::check() output:

* checking files in ‘vignettes’ ... WARNING
Files in the 'vignettes' directory but no files in 'inst/doc':
  ‘rppo-vignette.Rmd’, ‘rppo-vignette.md’
Files named as vignettes but with no recognized vignette engine:
   ‘vignettes/rppo-vignette.Rmd’
(Is a VignetteBuilder field missing?)

Similarly, browseVignettes() fails to find the document.

> browseVignettes("rppo")
No vignettes found by browseVignettes("rppo")

For now I haven't checked the vignette box in the checklist but will do as soon as this is resolved.

In terms of content, the vignette could include a more fleshed out example of how the two functions can be combined to produce additional insights. I imagine they will often be used together?

Functions

Tests

goodpractice::gp() suggests a couple of small changes:

A lone typo (well done!) found by devtools::spell_check(): in ppo_terms.Rd row 20 "pouplates" instead of "populates".

I hope this is useful, thank you very much for your work on this!

tdjames1 commented 6 years ago

Likewise, here's mine!

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:

For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [ ] A short summary describing the high-level functionality of the software
  • [ ] Authors: A list of authors with their affiliations
  • [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

Final approval (post-review)

Estimated hours spent reviewing: 5


Review Comments

rppo provides a method of accessing a web data portal to query records of plant phenology observations from North America and Europe. It will be of use to scientists who wish to access global data on plant phenology. While the package is simple to use, it would benefit from amendments to its documentation to clarify its purpose and usage. In particular:

I have detailed below a few further points with reference to the ROpenSci package guidelines.


README
DESCRIPTION

checking DESCRIPTION meta-information ... WARNING Dependence on R version ‘3.4.2’ not with patchlevel 0

Do you need to depend on this particular patchlevel? From https://cran.r-project.org/doc/manuals/r-patched/R-exts.html#Package-Dependencies:

It is inadvisable to use a dependence on R with patchlevel (the third digit) other than zero. Doing so with packages which others depend on will cause the other packages to become unusable under earlier versions in the series, and e.g. versions 3.x.1 are widely used throughout the Northern Hemisphere academic year.

DOCUMENTATION

rppo

ppo_terms

Description

I suggest keeping the description concise, e.g. "Retrieve terms from the Plant Phenology Ontology", and moving the rest of the information to a "Details" section.

Fix typo: pouplates

ppo_data

Description

This section should describe the function specifically. The paragraph provided is background information that could go in a "Details" section and/or in the package documentation.

Arguments

termID - suggest cross referencing ppo_terms; "to get the relevant IDs for present and absent stages" is not very clear without a fuller description of the function.
fromYear/toYear/fromDay/toDay - indicate parameter format (numeric?); maybe reword, e.g. "return data starting from the specified year".
bbox - the argument description here is too long and unclear. I suggest that the information for creating the bounding box is clarified and moved to a "Details" section of the function documentation. Also, indicate that it's a string value.

Value

I suggest describing the return value explicitly, e.g.

A list containing the following components:
`data`      [describe data component]
`readme`    [etc]
`citation`

Fix typo: resultset

Vignette

Files in the 'vignettes' directory but no files in 'inst/doc': ‘rppo-vignette.Rmd’, ‘rppo-vignette.md’

NEWS
TESTING

ppo_data

ppo_terms

EXAMPLES

ppo_data

PACKAGE DEPENDENCIES
SCAFFOLDING

For parsing JSON, use jsonlite instead of rjson

CONSOLE MESSAGES

Use message() and warning() to communicate with the user in your functions. Please do not use print() or cat() unless it's for a print.*() method, as these methods of printing messages are harder for the user to suppress.

CRAN GOTCHAS
Coding style
Code duplication

Not applicable.

User interface issues

Should the data component returned from ppo_data include information about the termIDs associated with the individual observations?

Performance issues

No performance issues noted.

BUGS

ppo_data.R line 115/117 :<- should be :<=

annakrystalli commented 6 years ago

Thanks @remsamp & @tdjames1! 🙌

@jdeck88 both reviews are now in for your consideration. 👍

annakrystalli commented 6 years ago

@jdeck88 I had also forgotten to ask you to add the rOpenSci review badge to the README! 😬 Could I ask to do this now, please? markdown snippet attached below.

[![](https://badges.ropensci.org/175_status.svg)](https://github.com/ropensci/onboarding/issues/207)
jdeck88 commented 6 years ago

Thank you for the reviews! I will go through these in the next week.

jdeck88 commented 6 years ago

Thank you for your reviews. They were very helpful! I have addressed your comments... In most cases i was able to make updates, which are listed under UPDATES below. In some cases i had comments or questions and i've listed these under QUESTIONS/COMMENTS.

UPDATES:

Reviewer @remsamp

Reviewer @tdjames1

QUESTIONS/COMMENTS:

Reviewer @remsamp

Reviewer @tdjames1 :

annakrystalli commented 6 years ago

Thanks for your response @jdeck88!

@tdjames1, @remsamp please let us know if there any further comments/responses to @jdeck88's response or whether you're happy that all issues raised have been appropriately dealt with.

tdjames1 commented 6 years ago

Hi @jdeck88, thanks for the thorough response. The documentation is much improved and the vignette is more helpful now, I think. You could include a full reference to your paper in the vignette rather than just a link (see https://rmarkdown.rstudio.com/authoring_bibliographies_and_citations.html).

Running goodpractice::gp() threw up a couple of further points that have popped up in things that have been changed. I'm flagging these up here as you may wish to address them before submitting to CRAN:

 ✖ 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 '<-'.

    tests/testthat/test-all.R:55:21
    tests/testthat/test-all.R:56:20
    tests/testthat/test-all.R:57:17

  ✖ 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/ppo_data.R:209:45

There still seems to be a typo in ppo_terms.R: line 8 "pouplates"; also I noticed a typo in the updated README.md: line 38 "reccomend".

Regarding URLs in the description field of DESCRIPTION file:

URLs should be enclosed in angle brackets, e.g. ‘\<https://www.r-project.org>’: see also Specifying URLs.

I don't know how to address the issues regarding building vignettes, maybe @annakrystalli can advise?

@annakrystalli: pending the few points above I'm satisfied that all issues have been addressed. Do I need to do anything further to sign this off?

jdeck88 commented 6 years ago

Responses inline and indicated in BOLD (with fixes committed to https://github.com/biocodellc/rppo)

You could include a full reference to your paper in the vignette rather than just a link (see https://rmarkdown.rstudio.com/authoring_bibliographies_and_citations.html).

I tried this and was able to render this in HTML but not in Markdown. I worked on this for awhile this afternoon but felt that a single reference in this simple vignette was not worth the effort to track down why the markdown version was not rendering properly so left the link

Running goodpractice::gp() threw up a couple of further points that have popped up in things that have been changed. I'm flagging these up here as you may wish to address them before submitting to CRAN:

✖ 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 '<-'.

tests/testthat/test-all.R:55:21
tests/testthat/test-all.R:56:20
tests/testthat/test-all.R:57:17

Fixed

✖ avoid sapply(), it is not type safe. It might return a vector, or a list, depending on the input data. Consider using vapply() instead.

Fixed

R/ppo_data.R:209:45

There still seems to be a typo in ppo_terms.R: line 8 "pouplates"; also I noticed a typo in the updated README.md: line 38 "reccomend".

Fixed

Regarding URLs in the description field of DESCRIPTION file:

URLs should be enclosed in angle brackets, e.g. ‘https://www.r-project.org’: see also Specifying URLs.

Ok, i figured that out

remsamp commented 6 years ago

Hi @jdeck88, apologies for the long time in getting back to you. I am happy with your responses to my initial comments, thank you! I can also confirm that the points raised by @tdjames1 have been addressed by your most recent commits.

To note: browseVignettes("rppo") now works after installing the package from github with devtools::install_github("biocodellc/rppo").

@annakrystalli: I am happy with the state the package has got to. Please do let me know if I need to do anything else. Thanks!

annakrystalli commented 6 years ago

Right, so that's a wrap! Thanks for all your work and input @tdjames1 & @remsamp!

@jdeck88, there's just a few last housekeeping steps to finalise the review process:

Any further questions, let me know!

maelle commented 6 years ago

👋 @jdeck88 I promise I'd transfer the repo and not keep it to myself! Sorry about the awkward worfklow.

jdeck88 commented 6 years ago

OK...

  1. i tried transferring to ropensci and that, as i was warned may not work, didn't work.
  2. tried transferring to @maelle https://github.com/maelle but that didn't work becasue the repo was under an organization
  3. Then transferred to my personal github repo @jdeck88 and transferred to @ maelle https://github.com/maelle and that worked!!

I haven't updated the travis CI links as i'll wait for it to become live at its final destination since it is moving around a bit right now.

I did though add the ropensci badge to the bottom of the readme. John

On Thu, May 31, 2018 at 8:36 AM, Maëlle Salmon notifications@github.com wrote:

👋 @jdeck88 https://github.com/jdeck88 I promise I'd transfer the repo and not keep it to myself! Sorry about the awkward worfklow.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/onboarding/issues/207#issuecomment-393572256, or mute the thread https://github.com/notifications/unsubscribe-auth/ABGdxX8XGMwieiWGsvxhuyTwjPALtvYYks5t4A3pgaJpZM4TCTAN .

-- John Deck (541) 914-4739

maelle commented 6 years ago

@jdeck88 your repo is now in ropensci and I made you admin again 🎊 thanks for your trust :wink:

annakrystalli commented 6 years ago

Great @jdeck88! Thanks again for submitting. I hope you found the process constructive and congratulations on your newly minted rOpenSci package! 🙌

jdeck88 commented 6 years ago

Thanks Anna... i found the process constructive!

one last thing... is there something i need to change the badge from "in review" to "peer reviewed"?

annakrystalli commented 6 years ago

Good catch @jdeck88. My understanding was that this should update automatically when the issue is closed. @karthik any advice?

jdeck88 commented 6 years ago

Figured this out... turns out i had the wrong issue # in the badge status img link (but the correct one for the hyperlink!)