rotl (Open Tree of Life)

[fmichonneau]

• 1. What does this package do? (explain in 50 words or less)

It interacts with the Open Tree of Life API (http://opentreeoflife.org/)

• 2. Paste the full DESCRIPTION file inside a code block (bounded by ``` on either end).

Package: rotl
Title: Interface to the Open Tree of Life API
Version: 0.1
Authors@R: c(
  person("Francois", "Michonneau", role=c("aut", "cre"),
         email="francois.michonneau@gmail.com"),
  person("Joseph", "Brown", role="aut"),
  person("David", "Winter", role="aut"))
Description: An interface to the Open Tree of Life API to retrieve phylogenetic
   trees, information about studies used to assemble the synthetic tree, and
   utilities to match taxonomic names to Open Tree identifiers. The Open Tree of
   Life aims at assembling a comprehensive phylogenetic tree for all named
   species.
URL: https://github.com/fmichonneau/rotl
BugReports: https://github.com/fmichonneau/rotl/issues
Depends:
    R (>= 3.1.1)
Imports:
    httr,
    jsonlite,
    rncl,
    ape
License: BSD_2_clause + file LICENSE
Suggests:
    knitr,
    rmarkdown,
    testthat,
    RNeXML
VignetteBuilder: knitr
LazyData: true

• 3. URL for the package (the development repository, not a stylized html page)

https://github.com/fmichonneau/rotl

• 4. What data source(s) does it work with (if applicable)?

http://api.opentreeoflife.org/

• 5. Who is the target audience?

Scientists who want to use phylogenies

• 6. Are there other R packages that accomplish the same thing? If so, what is different about yours?

No.

• 7. Check the box next to each policy below, confirming that you agree. These are mandatory.
• This package does not violate the Terms of Service of any service it interacts with.
• The repository has continuous integration with Travis and/or another service
• The package contains a vignette
• The package contains a reasonably complete readme with devtools install instructions
• The package contains unit tests
• The package only exports functions to the NAMESPACE that are intended for end users
• 8. Do you agree to follow the rOpenSci packaging guidelines? These aren't mandatory, but we strongly suggest you follow them. If you disagree with anything, please explain.
• Are there any package dependencies not on CRAN?
• Do you intend for this package to go on CRAN?
• Does the package have a CRAN accepted license?
• Did devtools::check() produce any errors or warnings? If so paste them below.
• 9. Please add explanations below for any exceptions to the above:
• 10. If this is a resubmission following rejection, please explain the change in cirucmstances.

[sckott]

• @sckott doing one review
• looking for another person...

[sckott]

Nice package! Overall, this is great! Some suggestions for improvements detailed below, as well as some ideas to ponder (but not necessarily to change anything).

DESCRIPTION file

CRAN will likely complain about some of the words in your Title and Description fields - any that are not normal dictionary words should get single quotes around them.

Installation

Seems pretty easy on all platforms, windows and linux installs pass.

Imports

• You have httr and jsonlite as Imports in DESCRIPTION file, but they aren't listed in any @import. Was there a reason behind this? Even though httr and jsonlite are namespaced, those will still fail for users that don't have those pkgs installed.
• You import all functions of your dependencies. It would be cleaner to use @importFrom and import only those functions you need instead of all of them.

Docs

• Some examples in this vignette http://dwinter.github.io/rotl-vignette/ don't work. And it is linked to from the README, so would be good if that vignette works. Looks like the package vignette doesn't have this issue though. At least the following is broken:

tr_string <- get_study_tree(study="2655", tree="tree6182", "newick")
#>  Error in match.arg(object_format) : 'arg' should be one of “phylo” 

Tests

All tests pass.

Examples

Could use more examples for each function. Many only have 1 example. Especially - man pages for grouped functions often don't have examples for most of the functions in the man file - though maybe there is a reason for this?

Function Documentation

• You have for tnrs_match_names for the ... parameter

additional arguments to customize the API request (see rotl package documentation).

And same for other function man files. But going to the linked page doesn't talk about what the ... actually does. Drilling down, looks like the ... gets passd on to httr HTTP methods, correct?

• Just a minor comment - for documentation bits that are duplicated, you could put documentation in .R files in a man-roxygen folder in the root of your repo, then use @template filename to use the docs.

Specific comments about the scripts and functions:

• ott_id vs. node_id - I see a potential addition here. I imagine some users could get these confused since they are both short numeric values, and often the same length? The functions that accept these ids as parameters arguments could have in addition generic S3 methods that act on each of them appropriately. So if you had functions that created S3 classes for each of ott_id vs. node_id, then functions could act on those correctly given their class. This is just brainstorming - don't feel compelled to do this, but something to think about at least.

• Fail behavior:

  • Below I wonder if this could fail better? Does the TOL API check that parameter inputs are of the correct type? If so, could pass those error messages along to use. If not, could check these yourself. I think most functions behave this way. I realize this exact mistake is not super likely, but better to program defensively.

  tol_subtree(ott_id = "adfasdf")
  #> Error: HTTP failure: 400

Readme:

First find ott ids for a set of names:

What is an ott?

Also, in readme travis badge noticed that Travis builds for mashup branch are failing. Looks like perhaps ape needs to be loaded in the vignette. Not a big deal as not master.

Code of conduct:

• Nice, thanks for adding this!

Documenting package changes

• I'd encourage authors to use a NEWS file to track changes, either as changes are made, or before new releases to CRAN. e.g., with links to issues (https://github.com/hadley/httr/blob/master/NEWS.md). In addition, do tag each release to CRAN, and paste in associated items in your NEWS file into a release for that tag, e.g., https://github.com/hadley/httr/releases/tag/v1.0.0

[fmichonneau]

Thanks for this awesome and detailed feedback! I'm currently at the
evolution conference but will incorporate your suggestions soon.
On Jun 29, 2015 3:17 PM, "Scott Chamberlain" notifications@github.com
wrote:

Nice package! Overall, this is great! Some suggestions for improvements
detailed below, as well as some ideas to ponder (but not necessarily to
change anything).
DESCRIPTION file

CRAN will likely complain about some of the words in your Title and
Description fields - any that are not normal dictionary words should
get single quotes around them.
Installation

Seems pretty easy on all platforms, windows and linux installs pass.
Imports

• You have httr and jsonlite as Imports in DESCRIPTION file, but they
  aren't listed in any @import. Was there a reason behind this? Even
  though httr and jsonlite are namespaced, those will still fail for
  users that don't have those pkgs installed.
• You import all functions of your dependencies. It would be cleaner
  to use @importFrom and import only those functions you need instead of
  all of them.

Docs

• Some examples in this vignette
  http://dwinter.github.io/rotl-vignette/ don't work. And it is linked
  to from the README, so would be good if that vignette works. Looks like the
  package vignette doesn't have this issue though. At least the following is
  broken:

tr_string <- get_study_tree(study="2655", tree="tree6182", "newick")#> Error in match.arg(object_format) : 'arg' should be one of “phylo”

Tests

All tests pass.
Examples

Could use more examples for each function. Many only have 1 example.
Especially - man pages for grouped functions often don't have examples for
most of the functions in the man file - though maybe there is a reason for
this?
Function Documentation

• You have for tnrs_match_names for the ... parameter

  additional arguments to customize the API request (see rotl package
  documentation).

And same for other function man files. But going to the linked page
doesn't talk about what the ... actually does. Drilling down, looks like
the ... gets passd on to httr HTTP methods, correct?

• Just a minor comment - for documentation bits that are duplicated,
  you could put documentation in .R files in a man-roxygen folder in the
  root of your repo, then use @template filename to use the docs.

Specific comments about the scripts and functions:

• ott_id vs. node_id - I see a potential addition here. I imagine some
  users could get these confused since they are both short numeric values,
  and often the same length? The functions that accept these ids as
  parameters arguments could have in addition generic S3 methods that act on
  each of them appropriately. So if you had functions that created S3 classes
  for each of ott_id vs. node_id, then functions could act on those
  correctly given their class. This is just brainstorming - don't feel

  compelled to do this, but something to think about at least.

  Fail behavior:

  • Below I wonder if this could fail better? Does the TOL API check
    that parameter inputs are of the correct type? If so, could pass those
    error messages along to use. If not, could check these yourself. I think
    most functions behave this way. I realize this exact mistake is not super
    likely, but better to program defensively.

  tol_subtree(ott_id = "adfasdf")#> Error: HTTP failure: 400

Readme:

First find ott ids for a set of names:

What is an ott?

Also, in readme travis badge noticed that Travis builds for mashup branch
are failing https://travis-ci.org/fmichonneau/rotl/builds/67776745#L1638.
Looks like perhaps ape needs to be loaded in the vignette. Not a big deal
as not master.
Code of conduct:

• Nice, thanks for adding this!

Documenting package changes

• I'd encourage authors to use a NEWS file to track changes, either as
  changes are made, or before new releases to CRAN. e.g., with links to
  issues (https://github.com/hadley/httr/blob/master/NEWS.md). In
  addition, do tag each release to CRAN, and paste in associated items in
  your NEWS file into a release for that tag, e.g.,
  https://github.com/hadley/httr/releases/tag/v1.0.0

—
Reply to this email directly or view it on GitHub
#17 (comment).

[fmichonneau]

Thanks so much for this review @sckott. It's very useful and informative!

I created issues for most major points and referenced them below so you can see how I addressed your comments by looking at the associated commits/diffs.

The current remaining issue is fixing the "mash-up" vignette.

DESCRIPTION file

CRAN will likely complain about some of the words in your Title and Description fields - any that are not normal dictionary words should get single quotes around them

Thanks for pointing this out, I quoted 'Open Tree of Life' and 'Open Tree identifiers', see ropensci/rotl#34

Imports

It would be cleaner to use @importFrom and import only those functions you need instead of all of them.

Done. See ropensci/rotl#35

Docs

Some examples in this vignette http://dwinter.github.io/rotl-vignette/ don't work.

Yes, @dwinter is working on fixing it. We are keeping track of this at ropensci/rotl#23

Examples

Could use more examples for each function. Many only have 1 example.

I added a few more examples, see ropensci/rotl#36

Especially - man pages for grouped functions often don't have examples for most of the functions in the man file - though maybe there is a reason for this?

Do you have any specific examples for this? I can think of taxonomy-methods.Rd that doesn't have any examples but individual methods have examples in other places. For instance, synonyms has examples in synonyms.match_names.Rd and taxonomy_info.Rd.

Function documentation

the linked page doesn't talk about what the ... actually does.

I tried to clarify the text in the documentation for the package, see ropensci/rotl#37. Let me know if it needs to be clarified further.

for documentation bits that are duplicated, you could put documentation in .R files in a man-roxygen folder

Cool. I didn't know about this. I'll definitely use it the future!

Specific comments about the scripts and functions:

ott_id vs. node_id

I completely removed the ability for users to use node_id. Apparently, those are not meant to be used for anything but internally by the neo4j database. They will be removed from the API in the next version, so users can only interact with the API using ott_id now. See ropensci/rotl#41.

Below I wonder if this could fail better?

Agreed. I had started doing this for other functions but I had missed a few. It should be fixed and fail more elegantly. See ropensci/rotl#38

tol_subtree(ott_id = "fjklqem")
# Error in .tol_subtree(ott_id = ott_id, tree_id = tree_id, ...) : 
#  ‘ott_id’ needs to look like a number.

README

What is an ott id?

Thanks for pointing it out. Tried to clarify: see ropensci/rotl#43

that Travis builds for mashup branch are failing

This goes with fixing the other vignette. It should be resolved soon.

NEWS file

That sounds good. I'll add a NEWS file when preparing for the CRAN release.

[dwinter]

Hi both, thanks @sckott for your time reviewing this and @fmichonneau for powering through these issues!

Just a note on the "mashup" vignette. It's going to be a little harder than I thought -- the tree I used in the earlier version has disappeared from OpenTree and a quick look at alternatives found less-than-great examples.

Do you have a time-line for CRAN submission / rOpenSci conclusion? I will try and work on this today, but might not have much time after that...

[sckott]

Do you have any specific examples for this? I can think of taxonomy-methods.Rd that doesn't have any examples but individual methods have examples in other places...

Okay, I see what you mean, looks good.

WRT man-roxygen templates - make sure to put man-roxygen in .Rbuildignore

the linked page doesn't talk about what the ... actually does.

fixes look good

[sckott]

@dwinter no timeline on our end, that's up to @fmichonneau

[fmichonneau]

OK, I think we are good to go at this stage. I merged @dwinter's revamped vignette into master.

We may have to wait a few days for a CRAN release though, as I released the package rotl depends on (rncl) a few days ago. Unfortunately, in that release, while fixing a bug, I introduced stricter checks on the validity of the files. These checks made reading some OpenTree trees impossible. I resolved the issue in rncl but I don't know if it's wise to resubmit rncl to CRAN just 5 days following the previous submission. Do you have any advice on this?

Otherwise, if you are satisfied with the current status of rotl, let us know of the next step.

Thanks.

cc @josephwb

[sckott]

@fmichonneau sorry for the delay, just got back from 1 week vacation. i'll get to next steps soon...

[sckott]

@fmichonneau in terms of submitting rncl again to CRAN, they generally do get more annoyed the more often you submit a new version of a package, but it's worth a shot, and I think they are more lenient with people that maintain fewer packages (i.e., they don't like me)

[fmichonneau]

Thanks for the feedback and no worries about the delay. I'll try to submit
an update to rncl this evening and I'll keep you posted.

On Mon, Jul 20, 2015 at 3:12 PM, Scott Chamberlain <notifications@github.com

wrote:

@fmichonneau https://github.com/fmichonneau in terms of submitting rncl
again to CRAN, they generally do get more annoyed the more often you submit
a new version of a package, but it's worth a shot, and I think they are
more lenient with people that maintain fewer packages (i.e., they don't
like me)

—
Reply to this email directly or view it on GitHub
#17 (comment).

[sckott]

@fmichonneau looks great to me, I marked as approved

Okay if we now move it into the ropensci organization account? If so, i've created a team in our org account and invited you to it. I think after you accept you'll be able to transfer the repo to ropensci

Also, make sure to update github installation instructions and any repo links

[fmichonneau]

I transfered the repository to ropensci. I'll update all the links later this evening. Thanks!

[sckott]

great! thanks