Skip to content
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

[REVIEW]: modeLLtest: An R Package for Unbiased Model Comparison using Cross Validation #1542

Closed
36 tasks done
whedon opened this issue Jul 2, 2019 · 67 comments
Closed
36 tasks done
Assignees
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review

Comments

@whedon
Copy link

whedon commented Jul 2, 2019

Submitting author: @ShanaScogin (Shana Scogin)
Repository: https://github.com/ShanaScogin/modeLLtest
Version: 1.0.1
Editor: @usethedata
Reviewers: @andrewheiss, @usethedata
Archive: 10.5281/zenodo.3377959

Status

status

Status badge code:

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

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

@monogan & @andrewheiss, 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 @usethedata know.

Please try and complete your review in the next two weeks

Review checklist for @usethedata

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: 1.0.1
  • Authorship: Has the submitting author (@ShanaScogin) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • 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

Software paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @andrewheiss

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: 1.0.1
  • Authorship: Has the submitting author (@ShanaScogin) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • 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

Software paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
@whedon
Copy link
Author

whedon commented Jul 2, 2019

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @monogan, @andrewheiss it looks like you're currently assigned to review this paper 🎉.

⭐ Important ⭐

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
Copy link
Author

whedon commented Jul 2, 2019

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Jul 2, 2019

@whedon whedon assigned ghost and andrewheiss Jul 3, 2019
@labarba
Copy link
Member

labarba commented Jul 29, 2019

👋 @monogan, @andrewheiss — This is where the action happens. You both have a review checklist at the top of this issue. Please work your way through it, adding your comments to the authors in this thread, and opening new issues on the submission repository as needed. Godspeed!

@usethedata
Copy link

Bumping issue. I've reached out to @monogan and @andrewheiss via email.

@ghost
Copy link

ghost commented Aug 5, 2019

Yes, I'm still glad to do this review. When is the due date?

@usethedata
Copy link

The target is 2-3 weeks of when the review issue opened. So, the sooner you can get to it, the better. Thanks.

@andrewheiss
Copy link

Ahhh, sorry! I moved across the country to start a new job and this got dropped from my todo list. I'll get to it ASAP.

@ShanaScogin
Copy link

Thank you all for reviewing/editing, especially during the busy summer! Let me know if there are any questions or clarifications. We look forward to the review.

@andrewheiss
Copy link

modeLLtest is a helpful new package that makes it easy to compare the performance of different models with cross-validated log-likelihood values. It includes two real-world datasets to accompany its examples, and it is fairly well documented. I came across a few issues, noted below. The biggest issue is that some objects are somewhat difficult to use since they're simple R lists without helper S3 functions, but this is easily addressed.

Installation

The package installed without a problem on macOS, Linux, and Windows, both from source and from CRAN.

The README includes a note about gfortran-related installation errors. I wasn't able to get any of these to appear. It might be helpful to include the wording of the gfortran error in the README, or potentially include directions for fixing the error in the README in case the linked repository or the linked blog post go away in the future.

Usage

The examples in the README and vignette run fine. It might be helpful to expand the examples a bit to (1) show how to extract the relevant information (even if it's just running x$best or x$test_stat, etc.), and (2) briefly show how to interpret the results and choose the best model. The JOSS paper helpfully goes over the technical details of why these tests and checks are important, but neither the paper nor the vignette/README show an example of how to choose which model is the best. Not knowing the statistical papers that the authors reference, I had to figure out which model was best by inspecting the resulting object and discovering the $best slot. Without reviewing the referenced papers, I'm not quite sure how to interpret that—though I trust that it really is the best :)

Relatedly, it might be very helpful to provide helper functions for extracting the relevant information out of the cvdm-classed objects. This could be something like an S3 print.cvdm class that shows $best, $test_stat, and $p_value more clearly, or some other custom function. Right now, printing a cvlldiff object is okay, since it only has a few slots in the list and everything important is readable on one screen, but printing cvdm objects hides the relevant information since it's buried under less important list slots like $call, $x, and $y.

The duration of some models caught me by surprise. Running the earlier models in the README file (obj_cvdm <- cvdm(Y ~ X, data.frame(cbind(Y, X)), method1 = "OLS", method2 = "MR") for instance) was more or less instantaneous, but it took 100 seconds to run the obj_cvmf_golder cvmf() example. Noting that the model might take a while to fit in a comment in the example might be helpful. The JOSS paper states that "these tests now run in a reasonable amount of time on large data sets," but it'd be nice to know what "reasonable" means. Two minutes might be reasonable if these tests ordinarily take longer without the C++ code, but without a baseline, it's hard to tell.

Documentation

It might be helpful to link to the documentation for cvdm_object() within the man pages for cvdm() and cvll().

Contributions

It might be helpful to have some community guidelines in the package too, such as a CONTRIBUTING.md file (perhaps modeled after something like this or this) and a CONDUCT.md file (like this or this)

Creating an RStudio project .Rproj file could be helpful for encouraging package contributions, since it would enable RStudio's package building interface.

Right now, according to the README, bug tracking is handled via e-mail: "If you encounter a bug or have comments or questions, please email me at [email protected]." It might be good to handle bugs more publicly through GitHub's issue tracker and include a BugReports: entry in DESCRIPTION. The startup message already directs users to the GitHub repository for development details—tracking issues there might be easiest and more community oriented.

Paper

The JOSS paper is clear and easy to follow, establishes a clear need for the functions, and includes DOIs in all the references.


That's all I have. Great work!

@ShanaScogin
Copy link

@andrewheiss Thank you for your time and comments! I've implemented the suggestions in the documentation and contributions sections and have begun addressing the suggestions in usage. Thank you again for your time - we appreciate the feedback!

@usethedata
Copy link

@andrewheiss Thanks much for your thoughtful review.

@ShanaScogin
Copy link

Hi all - We made a slight edit in a title of a journal in the paper so I am going to generate a new one. Let me know if you have any questions!

@ShanaScogin
Copy link

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Aug 13, 2019

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Aug 13, 2019

@usethedata
Copy link

@whedon remove @monogan as reviewer

@whedon whedon assigned andrewheiss and usethedata and unassigned andrewheiss, usethedata and ghost Aug 20, 2019
@whedon
Copy link
Author

whedon commented Aug 20, 2019

OK, @monogan is no longer a reviewer

@usethedata
Copy link

@whedon add @usethedata as reviewer

@whedon
Copy link
Author

whedon commented Aug 26, 2019

OK. 1.0.1 is the version.

@usethedata
Copy link

@whedon check references

@whedon
Copy link
Author

whedon commented Aug 26, 2019

Attempting to check references...

@whedon
Copy link
Author

whedon commented Aug 26, 2019


OK DOIs

- 10.1007/s11135-013-9884-7 is OK
- 10.1177/1532440011408929  is OK
- 10.1198/073500102753410444 is OK
- 10.1093/pan/mpr042  is OK
- 10.2307/1912557 is OK
- 10.2307/2669248 is OK
- 10.1177/0022002702239512  is OK
- 10.1093/pan/mpm004 is OK
- 10.1017/CBO9780511754098 is OK
- 10.1177/0010414009341714 is OK
- 10.1177/0022343308096155 is OK
- 10.1080/01621459.1978.10480051 is OK

MISSING DOIs

- None

INVALID DOIs

- None

@usethedata
Copy link

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Aug 26, 2019

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Aug 26, 2019

@usethedata
Copy link

@openjournals/joss-eics I've completed my checks and recommend publication.

@labarba
Copy link
Member

labarba commented Sep 1, 2019

Yikes, I missed this notification in the email avalanche. Sorry about that!

@labarba
Copy link
Member

labarba commented Sep 1, 2019

@ShanaScogin — I don't see a tagged release in the software repository to match the version associated with the JOSS paper. Could you issue a release?

@labarba
Copy link
Member

labarba commented Sep 1, 2019

Also, the final paragraph of your paper says that "the code is on GitHub" with a link. Please remove this. The link to the software repository and archive are both included in the margin decorators on the front page of the paper.

@ShanaScogin
Copy link

@labarba Thanks for the help. I've deleted that sentence and added release 1.0.1. Is there something else that needs to be done?

@labarba
Copy link
Member

labarba commented Sep 1, 2019

@whedon accept

@whedon
Copy link
Author

whedon commented Sep 1, 2019

Attempting dry run of processing paper acceptance...

@whedon
Copy link
Author

whedon commented Sep 1, 2019

Check final proof 👉 openjournals/joss-papers#937

If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#937, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true

@labarba
Copy link
Member

labarba commented Sep 1, 2019

@whedon accept deposit=true

@whedon whedon added the accepted label Sep 1, 2019
@whedon
Copy link
Author

whedon commented Sep 1, 2019

Doing it live! Attempting automated processing of paper acceptance...

@whedon
Copy link
Author

whedon commented Sep 1, 2019

🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦

@whedon
Copy link
Author

whedon commented Sep 1, 2019

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited 👉 Creating pull request for 10.21105.joss.01542 joss-papers#938
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01542
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

Any issues? notify your editorial technical team...

@labarba
Copy link
Member

labarba commented Sep 1, 2019

Congratulations, @ShanaScogin, your JOSS paper is published! 🚀

Big thanks to our editor: @usethedata, and reviewers: @andrewheiss, @usethedata — thank you for your contributions to JOSS 🙏

@labarba labarba closed this as completed Sep 1, 2019
@whedon
Copy link
Author

whedon commented Sep 1, 2019

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](https://joss.theoj.org/papers/10.21105/joss.01542/status.svg)](https://doi.org/10.21105/joss.01542)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.01542">
  <img src="https://joss.theoj.org/papers/10.21105/joss.01542/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.01542/status.svg
   :target: https://doi.org/10.21105/joss.01542

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

@ShanaScogin
Copy link

Thank you all! We appreciate everyone's help, comments, and support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review
Projects
None yet
Development

No branches or pull requests

5 participants