search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
696 stars 36 forks source link

[REVIEW]: qtl2pleio: Testing pleiotropy vs. separate QTL in multiparental populations #1435

Closed whedon closed 5 years ago

whedon commented 5 years ago

Submitting author: @fboehm (Frederick Boehm) Repository: https://github.com/fboehm/qtl2pleio Version: v1.0.3 Editor: @csoneson Reviewer: @vjcitn, @rcorty Archive: 10.5281/zenodo.3263924

Status

status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/66bca5dc3d2e72b6259159bad07aafaf"><img src="http://joss.theoj.org/papers/66bca5dc3d2e72b6259159bad07aafaf/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/66bca5dc3d2e72b6259159bad07aafaf/status.svg)](http://joss.theoj.org/papers/66bca5dc3d2e72b6259159bad07aafaf)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@vjcitn & @rcorty, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @csoneson know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @vjcitn

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @rcorty

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @vjcitn, it looks like you're currently assigned as the reviewer for this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands
whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

csoneson commented 5 years ago

@vjcitn, @rcorty - please perform your review in this issue, by updating your checklist above and posting comments (also see the additional instructions in the posts above). If you have any questions, don't hesitate to ping me.

rcorty commented 5 years ago

I'm seeing a 2-page pdf. Should I be seeing more?

rcorty commented 5 years ago

license file doesn't seem detailed enough

rcorty commented 5 years ago

In the summary, the authors write "For a pair of traits that map to a single genomic region, they formulated the test with the null hypothesis being pleiotropy (the two traits are affected by a single QTL) against the alternative hypothesis of two separate QTL, with each affecting exactly one trait in the pair."

QTL = quantitative trait locus, and it seems like there is only one locus being discussed. Would it be clearer to say "...the alternative hypothesis of distinct causal variants..."

rcorty commented 5 years ago

To make it easy for people to go through your README, consider making probs and pr available somehow, so users don't have to computer them -- these computations are not relevant for qtl2pleio per se -- they are relevant to the qtl2 framework.

csoneson commented 5 years ago

@rcorty Have a look here for the expected format of a JOSS submission (including the paper itself): https://joss.readthedocs.io/en/latest/review_criteria.html

vjcitn commented 5 years ago

I found it easy to install the package and run two of the vignettes. I did not attempt the "bootstrap cluster" vignette, which has references to HTCondor and SQUID ... it seems informative for users in a specific sort of environment. The provision of bootstrap realizations from the legacy analyses is helpful for readers aiming to understand the significance procedure.

I think it would be helpful to create print methods for the S3 classes that typically have large volumes of information associated with them. Print methods that summarize structure and aspects of content prevent long screendumps when users want to poke into an intermediate calculation.

I have little knowledge of C++. However I find it strange that this package src/ needs to define C++ methods for matrix x matrix and matrix x vector. These operations are not already available in standard libraries?

Package build is slow, so I attempted to run the unit tests by hand, and ran into

> test_that("find_lin_indep_cols works", {
+ 
+   set.seed(20151130)
+   x <- cbind(1, sample(0:1, 200, repl=TRUE))
+ 
+   expect_equal(sort(find_lin_ .... [TRUNCATED] 
Error: Test failed: 'find_lin_indep_cols works'
* could not find function "find_lin_indep_cols"
1: expect_equal(sort(find_lin_indep_cols(x)), c(1, 2)) at test-drop_depcols.R:12
2: quasi_label(enquo(object), label, arg = "object")
3: eval_bare(get_expr(quo), get_env(quo))
4: sort(find_lin_indep_cols(x))

[Added after first comment was filed.] I ran R CMD check on qtl2pleio 0.1.3 and found

* checking dependencies in R code ... NOTE
Namespaces in Imports field not imported from:
  ‘RcppEigen’ ‘broman’ ‘devtools’ ‘microbenchmark’ ‘mvtnorm’ ‘progress’
  ‘qtl2’ ‘readr’
...
--- re-building ‘recla-analysis.Rmd’ using rmarkdown
trying URL 'https://zenodo.org/record/2652931/files/recla-aprobs.rds?download=1'
Quitting from lines 51-56 (recla-analysis.Rmd) 
Error: processing vignette 'recla-analysis.Rmd' failed with diagnostics:
cannot open URL 'https://zenodo.org/record/2652931/files/recla-aprobs.rds?download=1'
--- failed re-building ‘recla-analysis.Rmd’

[End of addition.] By and large I consider this a fine OSS project and consider the review complete. Let me know if more information is needed.

csoneson commented 5 years ago

👋 Hi all - just checking in on how it's going. @vjcitn - thanks for your review! @rcorty - do you have additional comments for the authors at this stage, or do the ones given above cover the unchecked boxes in your review checklist? @fboehm - please take a look at the comments and report back here whenever you are ready for the reviewers to have another look! Of course, don't hesitate to ping me if you have questions.

fboehm commented 5 years ago

Thanks @csoneson and @vjcitn and @rcorty!

I believe that there was a temporary problem with the zenodo website. That should explain the difficulty in rendering the vignette that @vjcitn mentions.

@vjcitn, I was unable to reproduce the error from the tests about 'find_lin_indep_cols'. The tests succeeded when I ran devtools::test(). Should I be running tests with a different function call? I did see that I omitted a call to library(qtl2pleio) in the test file, test-drop_depcols.R. I fixed this omission. I was under the impression that loading packages (for use in testing) was needed only in the file testthat.R.

I'm unsure why R CMD check gives the NOTE that you mention. It seems that some of the packages that you list are, in fact, not used by my package. I have now deleted from the DESCRIPTION file's Imports field those packages that I don't use in qtl2pleio. However, some of the packages that your output lists are used in qtl2pleio: broman and devtools are two examples. I will look into this to try to understand if I've miscoded something.

I haven't yet undertaken the development of print methods for the objects that qtl2 and qtl2pleio functions create, but I think that it's a good idea. I will include them in a future version.

Regarding the definition of matrix x matrix and matrix x vector multiplications in C++ files, I took these lines of C++ code from the qtl2 package. I believe that existing C++ packages (libraries) define them in a slightly different fashion than what is used in my package. Specifically, my code gives errors when inputs' dimensions don't match. I don't think that the C++ libraries' default errors are as clear.

@rcorty, I haven't yet responded to your comments. I intend to do so very soon. Thanks again!

fboehm commented 5 years ago

@vjcitn, when I run R CMD check on the tar.gz file, I get a note like the one that you get, about the Imports field of the DESCRIPTION file. I have been adding to Imports field packages that I use in vignettes, even if I don't use them in my qtl2pleio functions. Should I not do this?

vjcitn commented 5 years ago

On Wed, May 22, 2019 at 11:39 AM Frederick Boehm notifications@github.com wrote:

@vjcitn https://github.com/vjcitn, when I run R CMD check on the tar.gz file, I get a note like the one that you get, about the Imports field of the DESCRIPTION file. I have been adding to Imports field packages that I use in vignettes, even if I don't use them in my qtl2pleio functions. Should I not do this?

To my knowledge "Imports" concerns only code that will be compiled in the R folder of the package. Packages used only in vignettes need not be loaded on package use, so can be kept out of namespace processing. Whether this is stated in WRE or not I cannot say. You could ask on r-devel mailing list for certainty.

—

You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1435?email_source=notifications&email_token=ABDI5QREFUECSSUPQPEI7MLPWVSL5A5CNFSM4HLL3332YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV7O2AI#issuecomment-494857473, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDI5QS4RFGUU5O7KRTS54DPWVSL5ANCNFSM4HLL333Q .

-- The information in this e-mail is intended only for the person to whom it is addressed. If you believe this e-mail was sent to you in error and the e-mail contains patient information, please contact the Partners Compliance  HelpLine at http://www.partners.org/complianceline http://www.partners.org/complianceline . If the e-mail was sent to you in error but does not contain patient information, please contact the sender and properly dispose of the e-mail.

csoneson commented 5 years ago

I have been adding to Imports field packages that I use in vignettes, even if I don't use them in my qtl2pleio functions. Should I not do this?

@fboehm The "R packages" book by Hadley Wickham contains useful information on this topic: http://r-pkgs.had.co.nz/description.html. Also WRE: https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Package-Dependencies

vjcitn commented 5 years ago

from WRE: In particular, packages providing “only” data for examples or vignettes should be listed in ‘Suggests’ rather than ‘Depends’ in order to make lean installations possible.

On Wed, May 22, 2019 at 12:56 PM Charlotte Soneson notifications@github.com wrote:

I have been adding to Imports field packages that I use in vignettes, even if I don't use them in my qtl2pleio functions. Should I not do this?

@fboehm https://github.com/fboehm The "R packages" book by Hadley Wickham contains useful information on this topic: http://r-pkgs.had.co.nz/description.html. Also WRE: https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Package-Dependencies

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1435?email_source=notifications&email_token=ABDI5QW5V4EM54OC26ETLKDPWV3KRA5CNFSM4HLL3332YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV7VSWA#issuecomment-494885208, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDI5QXTLK2EHVCNCNOG5ODPWV3KRANCNFSM4HLL333Q .

-- The information in this e-mail is intended only for the person to whom it is addressed. If you believe this e-mail was sent to you in error and the e-mail contains patient information, please contact the Partners Compliance  HelpLine at http://www.partners.org/complianceline http://www.partners.org/complianceline . If the e-mail was sent to you in error but does not contain patient information, please contact the sender and properly dispose of the e-mail.

fboehm commented 5 years ago

Thanks, @csoneson and @vjcitn! I appreciate the help on this point. I'll fix the DESCRIPTION file accordingly.

fboehm commented 5 years ago

Hi, @rcorty, I've tried to address the issues that you raise in the comments as I've made revisions to the package. Thank you!

@csoneson - what are the next steps in the review process? Thanks again!

csoneson commented 5 years ago

@fboehm - great! To make things a bit easier for the reviewers, could you post a short summary of the changes that you have made in response to their comments? @vjcitn and @rcorty - could you please have a look at the revised version and see whether the changes address your comments?

fboehm commented 5 years ago

Thanks, @csoneson!

In response to @rcorty's comments, I made the following changes:

  1. added LICENSE.md file that contains the text of the MIT license.
  2. I tweaked the wording of the sentence in paper.md. However, I avoided using the term "causal variant".
  3. I didn't add precalculated genotype probabilities to the README. The reason for avoiding this is that the calculations in the README are done on only 3 chromosomes, and with a sparse marker set, so the calculations don't consume much computing time.

In response to @vjcitn's comments, I made these changes:

  1. Fixed the DESCRIPTION file - specifically the Suggests and Imports fields - so that the package passes R CMD check. I was previously ignorant of the distinction between these two fields. @vjcitn's references to Writing R Extensions were particularly helpful here.
  2. I added a line of R code - library(qtl2pleio) - to the specified test file to ensure that the test files can be run manually.
  3. I haven't yet implemented print methods for the objects that I create in the README and vignettes. I did open an issue on my repo for this. Since many of the print methods would apply to objects that are outputted by the qtl2 R package's functions, I might work with its authors to see about adding print methods to qtl2. Still, print methods for outputs from qtl2pleio-specific functions would be useful, too.
csoneson commented 5 years ago

👋@vjcitn, @rcorty - do you think you will have the chance to look at the revised submission in the coming week?

vjcitn commented 5 years ago

yes -- is it ready now

On Mon, Jun 10, 2019 at 3:04 PM Charlotte Soneson notifications@github.com wrote:

👋@vjcitn https://github.com/vjcitn, @rcorty https://github.com/rcorty

  • do you think you will have the chance to look at the revised submission in the coming week?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1435?email_source=notifications&email_token=ABDI5QRUGS4XMW62DFP7NYDPZ2QSRA5CNFSM4HLL3332YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXK4NPA#issuecomment-500549308, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDI5QTORAX3L6QLPPCKHGLPZ2QSRANCNFSM4HLL333Q .

-- The information in this e-mail is intended only for the person to whom it is addressed. If you believe this e-mail was sent to you in error and the e-mail contains patient information, please contact the Partners Compliance  HelpLine at http://www.partners.org/complianceline http://www.partners.org/complianceline . If the e-mail was sent to you in error but does not contain patient information, please contact the sender and properly dispose of the e-mail.

csoneson commented 5 years ago

@vjcitn - see @fboehm's last comment above for the changes that have been implemented in response to your comments.

csoneson commented 5 years ago

ping @vjcitn, @rcorty

👋@vjcitn, @rcorty - do you think you will have the chance to look at the revised submission in the coming week?

vjcitn commented 5 years ago

@csoneson I have just looked at the paper which seems fine. @fboehm I think it is somewhat problematic to have the recla.Rmd in the vignettes folder as it involves the user in a download of 250+MB from zenodo and will go slowly if one is running CMD check. You could place that particular vignette in a dedicated folder in /inst.

Our dialogue on C++ does not need to go on indefinitely but I would comment that reproducing source code from qtl2 may not be the best strategy. You are already using LinkingTo and you have qtl2 in Suggests. It may be best to take whatever steps are necessary (perhaps moving qtl2 to Imports) to reuse the compiled code from qtl2 instead of replicating source. This is a minor concern.

I consider these optional issues for the developer and am ready to endorse acceptance of this software. @csoneson let me know what else I need to do, if anything.

csoneson commented 5 years ago

Thanks @vjcitn! Could you please complete your checklist in the first issue comment? We'll wait to hear back from @rcorty, and then I'll take it from there.

vjcitn commented 5 years ago

I have looked at the checklist and discovered that the References criterion on DOIs is not fulfilled. For example, the Bates/Eddelbuettel paper has DOI http://dx.doi.org/10.18637/jss.v052.i05

vjcitn commented 5 years ago

It is also not clear to me that "Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support" has been explicitly addressed. The existence of Code of Conduct seems relevant, and presence in github implicitly addresses the concern. A comment from the author on this matter in the manuscript or README.md could settle this.

csoneson commented 5 years ago

@whedon check references

whedon commented 5 years ago 
Attempting to check references...
whedon commented 5 years ago 

OK DOIs

- None

MISSING DOIs

- https://doi.org/10.1007/978-0-387-75692-9_9 may be missing for title: Bootstrap methods: another look at the jackknife
- https://doi.org/10.1534/genetics.115.183624 may be missing for title: The dissection of expression quantitative trait locus hotspots
- https://doi.org/10.1038/nmeth.2848 may be missing for title: Efficient multivariate linear mixed model algorithms for genome-wide association studies
- https://doi.org/10.1534/genetics.107.080101 may be missing for title: Efficient control of population structure in model organism association mapping
- https://doi.org/10.1101/550939 may be missing for title: Testing pleiotropy vs. separate QTL in multiparental populations
- https://doi.org/10.32614/rj-2011-002 may be missing for title: testthat: Get started with testing
- https://doi.org/10.18637/jss.v052.i05 may be missing for title: Fast and elegant numerical linear algebra using the RcppEigen package

INVALID DOIs

- None
vjcitn commented 5 years ago

amazing! can it fix them too?

danielskatz commented 5 years ago

no :)

danielskatz commented 5 years ago

and it's not always correct - it's just suggestions for things a person needs to check

csoneson commented 5 years ago

@fboehm - as noted by @vjcitn, please add DOIs to your references and add some guidelines for potential contributors and users looking for help, and we can tick off those boxes. I have emailed @rcorty and I hope he will also be able to take a look at the revised submission shortly.

fboehm commented 5 years ago

Thanks, @csoneson and @vjcitn! I just pushed to Github the updated bib file (with dois). I double-checked the dois for the articles that whedon identified. I also added contributor guidelines to README.md.

csoneson commented 5 years ago

@whedon check references

whedon commented 5 years ago 
Attempting to check references...
whedon commented 5 years ago 

OK DOIs

- doi:10.1214/aos/1176344552 is OK
- https://doi.org/10.1534/genetics.115.183624 is OK
- https://doi.org/10.1038/nmeth.2848 is OK
- https://doi.org/10.1534/genetics.107.080101 is OK
- https://doi.org/10.1534/g3.119.400098 is OK
- https://doi.org/10.32614/rj-2011-002 is OK
- https://doi.org/10.18637/jss.v052.i05 is OK

MISSING DOIs

- https://doi.org/10.18637/jss.v052.i05 may be missing for title: Fast and elegant numerical linear algebra using the RcppEigen package

INVALID DOIs

- None
csoneson commented 5 years ago

@fboehm - please double-check. I think you have added the RcppEigen DOI to the wrong bib entry.

fboehm commented 5 years ago

yes, you're right. I just now pushed the fix.

csoneson commented 5 years ago

@whedon check references

whedon commented 5 years ago 
Attempting to check references...
whedon commented 5 years ago 

OK DOIs

- doi:10.1214/aos/1176344552 is OK
- https://doi.org/10.1534/genetics.115.183624 is OK
- https://doi.org/10.1038/nmeth.2848 is OK
- https://doi.org/10.1534/genetics.107.080101 is OK
- https://doi.org/10.1534/g3.119.400098 is OK
- https://doi.org/10.32614/rj-2011-002 is OK
- https://doi.org/10.18637/jss.v052.i05 is OK

MISSING DOIs

- https://doi.org/10.18637/jss.v052.i05 may be missing for title: Fast and elegant numerical linear algebra using the RcppEigen package

INVALID DOIs

- None
csoneson commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

csoneson commented 5 years ago

There are still a couple of missing DOIs. Please also check capitalization in the bibliography and add {} around letters if necessary.

fboehm commented 5 years ago

Thanks - I had missed a few DOIs. Do all articles have DOIs? I'm unsure how to get the DOIs for the 1995 articles.

fboehm commented 5 years ago

@csoneson - I now see the capitalization problems in the bibliography. Does adding braces around letters lead to no changes in the capitalization for those letters?