-
-
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
treeio for parsing and exporting phylogenetic trees #179
Comments
Hi @GuangchuangYu - Thanks very much for your submission. I'll report back with an editor check shortly |
Editor checks:
Editor commentsThanks for your submission @GuangchuangYu !!
It is good practice to
✖ write unit tests for all functions, and all package code
in general. 7% of code lines are covered by test cases.
R/AllGenerics.R:12:NA
R/AllGenerics.R:26:NA
R/ancestor.R:5:NA
R/ancestor.R:6:NA
R/ancestor.R:7:NA
... and 1326 more lines
✖ 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 '<-'.
R/hyphy.R:205:18
R/hyphy.R:207:18
R/jtree.R:66:20
R/jtree.R:68:21
R/jtree.R:69:22
... and 50 more lines
✖ 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/AllGenerics.R:26:1
R/beast.R:136:1
R/beast.R:199:1
R/beast.R:206:1
R/beast.R:239:1
... and 45 more lines
✖ 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/beast.R:71:14
R/beast.R:97:16
R/beast.R:233:21
R/beast.R:239:19
R/beast.R:279:11
... and 16 more lines
✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...),
1:NROW(...) and 1:NCOL(...) expressions. They are error prone and
result 1:0 if the expression on the right hand side is zero. Use
seq_len() or seq_along() instead.
R/method-groupOTU.R:61:19
R/write-beast.R:88:25
R/write-beast.R:160:15
tests/testthat/test-nhx.R:75:25 During check: * checking top-level files ... NOTE
Non-standard file/directory found at top level:
‘CONTRIBUTORS.md’ Seeking reviewers now 🕐 Reviewers: @uyedaj @fmichonneau |
thanks @sckott ! I have checked all the boxes you mentioned and add ropensci badge on README.md. |
current goodpractice output
|
reviewers assigned Reviewers: @uyedaj @fmichonneau |
sorry for the late review, I'll try to finish over the weekend. |
thanks @fmichonneau @uyedaj can you get your review in soon? |
I'm in the same boat, will try to finish over the weekend. |
thanks @uyedaj |
@fmichonneau @uyedaj please get your reviews in as soon as you can, thanks! 😸 |
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: 3 Review CommentsThis package covers an important and useful niche in the R phylogenetics ecosystem that greatly improves parsing tree formats from different phylogenetics software packages. It will no doubt be useful for researchers, and I will be happy to use it in my own work in the future. The package has very clear and useful vignettes using provided datasets that make it clear how the package is used. This is done at the expense of examples in the .Rd files, which are largely lacking. The package is seamlessly integrated into the packages When learning how to use the package, I immediately typed in the name of the object and noticed the The provided datasets are a bit hefty for an R package at 3.2MB. Perhaps understandably however, but There is a typo in What kind of multiphylo support is there in this package? I see that you can import a whole set of trees Overall, I find little to criticize in this excellent package! |
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: 3 Review CommentsDocumentation
InstallationDuring installation I see:
It also seems that the order in which FunctionalityIn the submission, the author indicates that no other packages have similar In general, it seems that an overview of where TestsIt looks like there are extensive tests for the code, but the codecov badge is currently broken. A quick look at the log files on Travis reveal that it's because the patch version for the minimum R version is listed in the README (it should be Minor comments
|
thanks for your review @fmichonneau ! can you give an estimate of how many hrs spent on the review? |
you're welcome and sorry for the delay. I updated my review to include the time estimate. Thanks |
@GuangchuangYu both reviews are now in |
thanks for the comments. I will address these issues asap. |
@uyedaj Thanks for your comments and here is my response.
Had been changed to
Since One solution I can think of is to reduce the size by
corrected.
|
Thanks for @fmichonneau 's comments and here is my response. Documentation
have updated and should be more easy to understand.
A session of
Installation
All these warning message had been removed.
When installing Functionality
Several packages listed in
There are many other formats that can not be imported directly into R and we developed Tests
fixed and the codecov badge is available. Minor comments
Corrected.
Corrected. |
thanks for your responses to reviewers @GuangchuangYu @uyedaj @fmichonneau let me know if you are happy with the maintainers responses - or feel free to keep the conversation going if you have further thoughts |
That looks good to me 🎉 |
@uyedaj did @GuangchuangYu address your concerns adequately? |
Yes, it looks good to me. Thanks. |
@GuangchuangYu both reviewers are okay with changes. I'll have a quick look myself now to see if there's any lingering issues. |
With the bioc versions of test-conversion.R:4: error: (unknown)
object ‘groupClade’ is not exported by 'namespace:treeio'
1: ggtree::ggtree at /Users/sckott/github/ropensci/treeio/tests/testthat/test-conversion.R:4
2: getExportedValue(pkg, name)
3: asNamespace(ns)
4: getNamespace(ns)
5: tryCatch(loadNamespace(name), error = function(e) stop(e))
6: tryCatchList(expr, classes, parentenv, handlers)
7: tryCatchOne(expr, names, parentenv, handlers[[1L]])
8: value[[3L]](cond)
test-treedata-accessor.R:34: error: (unknown)
object ‘groupClade’ is not exported by 'namespace:treeio'
1: ggtree::ggtree at /Users/sckott/github/ropensci/treeio/tests/testthat/test-treedata-accessor.R:34
2: getExportedValue(pkg, name)
3: asNamespace(ns)
4: getNamespace(ns)
5: tryCatch(loadNamespace(name), error = function(e) stop(e))
6: tryCatchList(expr, classes, parentenv, handlers)
7: tryCatchOne(expr, names, parentenv, handlers[[1L]])
8: value[[3L]](cond) Session InfoSession info ------------------------------------------------------------------
setting value
version R version 3.4.4 beta (2018-03-05 r74359)
system x86_64, darwin15.6.0
ui X11
language (EN)
collate en_US.UTF-8
tz America/Los_Angeles
date 2018-03-06
Packages ----------------------------------------------------------------------
package * version date source
ape 5.0 2017-10-30 CRAN (R 3.4.2)
assertthat 0.2.0 2017-04-11 CRAN (R 3.4.0)
base * 3.4.4 2018-03-06 local
bindr 0.1 2016-11-13 CRAN (R 3.4.0)
bindrcpp 0.2 2017-06-17 CRAN (R 3.4.0)
commonmark 1.4 2017-09-01 cran (@1.4)
compiler 3.4.4 2018-03-06 local
datasets * 3.4.4 2018-03-06 local
devtools 1.13.5 2018-02-18 CRAN (R 3.4.3)
digest 0.6.15 2018-01-28 CRAN (R 3.4.3)
dplyr 0.7.4 2017-09-28 CRAN (R 3.4.2)
glue 1.2.0 2017-10-29 CRAN (R 3.4.2)
graphics * 3.4.4 2018-03-06 local
grDevices * 3.4.4 2018-03-06 local
grid 3.4.4 2018-03-06 local
jsonlite 1.5 2017-06-01 CRAN (R 3.4.0)
lattice 0.20-35 2017-03-25 CRAN (R 3.4.4)
lazyeval 0.2.1 2017-10-29 cran (@0.2.1)
magrittr 1.5 2014-11-22 CRAN (R 3.4.0)
memoise 1.1.0 2018-02-27 Github (hadley/memoise@611cfad)
methods * 3.4.4 2018-03-06 local
nlme 3.1-131.1 2018-02-16 CRAN (R 3.4.4)
parallel 3.4.4 2018-03-06 local
pillar 1.2.1 2018-02-27 CRAN (R 3.4.3)
pkgconfig 2.0.1 2017-03-21 CRAN (R 3.4.0)
R6 2.2.2 2017-06-17 CRAN (R 3.4.0)
Rcpp 0.12.15 2018-01-20 CRAN (R 3.4.3)
rlang 0.2.0.9000 2018-03-06 Github (tidyverse/rlang@9ea33dd)
roxygen2 6.0.1 2017-02-06 CRAN (R 3.4.0)
rstudioapi 0.7 2017-09-07 CRAN (R 3.4.1)
rvcheck 0.0.9 2017-07-10 cran (@0.0.9)
stats * 3.4.4 2018-03-06 local
stringi 1.1.6 2017-11-17 CRAN (R 3.4.2)
stringr 1.3.0.9000 2018-03-06 Github (tidyverse/stringr@097c0c0)
testthat * 2.0.0 2017-12-13 CRAN (R 3.4.3)
tibble 1.4.2 2018-01-22 CRAN (R 3.4.3)
tidytree 0.1.7 2018-02-27 CRAN (R 3.4.3)
tools 3.4.4 2018-03-06 local
treeio * 1.3.12 <NA> Bioconductor
utils * 3.4.4 2018-03-06 local
withr 2.1.1.9000 2018-03-05 Github (jimhester/withr@5d05571)
xml2 1.2.0 2018-01-24 CRAN (R 3.4.3) |
@sckott The current bioc dev branch of both treeio and ggtree is identical to github version. The reason for the issue is that the OSX tarball of ggtree in Bioc dev is not updated due to the suggested package The The issue you presented here is on the Bioconductor side as |
install.packages("https://bioconductor.org/packages/devel/bioc/src/contrib/treeio_1.3.12.tar.gz", repo=NULL)
install.packages("https://bioconductor.org/packages/devel/bioc/src/contrib/ggtree_1.11.6.tar.gz", repo=NULL) After I installed the treeio and ggtree using the source tarball on Bioconductor, I clone the repo and run the
The error you presented here should be due to inconsistent versions (i.e. mix installation of release and devel branches), since there is a lot of change in the |
Thanks, works well. Approved! Thanks again for your submission @GuangchuangYu
|
@sckott thanks for accepting the pkg. All the requirements are incorporated in @stefaniebutland I think for the first time of introducing a package, a long-form post is more appropriate. |
Great to hear you're interested in contributing a post @GuangchuangYu! Here are some editorial and technical guidelines https://github.com/ropensci/roweb2#contributing-a-blog-post. You can have a look at other posts about onboarded software to see what style and length you like: https://ropensci.org/tags/review/ Would you be able to submit a draft via pull request 2018-04-03? This gives us an opportunity to review it, give you feedback and confirm a publication date. |
FYI, the |
Congratulations @GuangchuangYu! I added the citation as a comment on your blog post https://ropensci.org/blog/2018/05/17/treeio/ (I used to dream of publishing in MBE back when I was doing research) |
thanks @GuangchuangYu ! |
Summary
Parsing and exporting phylogenetic trees with associated data.
https://github.com/GuangchuangYu/treeio
data extraction
[e.g., "data extraction, because the package parses a scientific data file format"]
Researchers doing ecology and evolution.
yours differ or meet our criteria for best-in-category?
No.
No.
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:
No.
The text was updated successfully, but these errors were encountered: