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]: origami - Cross-validation Framework #512

Closed
18 tasks done
whedon opened this issue Dec 17, 2017 · 14 comments
Closed
18 tasks done

[REVIEW]: origami - Cross-validation Framework #512

whedon opened this issue Dec 17, 2017 · 14 comments
Assignees
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review

Comments

@whedon
Copy link

whedon commented Dec 17, 2017

Submitting author: @jeremyrcoyle (Jeremy Coyle)
Repository: https://github.com/jeremyrcoyle/origami
Version: v0.8
Editor: @karthik
Reviewer: @wrathematics
Archive: 10.5281/zenodo.1155901

Status

status

Status badge code:

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

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) 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

@wrathematics, 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.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @karthik know.

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: Does the release version given match the GitHub release (v0.8)?
  • Authorship: Has the submitting author (@jeremyrcoyle) 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 Dec 17, 2017

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @wrathematics it looks like you're currently assigned as the reviewer for 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 Dec 17, 2017

Attempting PDF compilation. Reticulating splines etc...

@whedon
Copy link
Author

whedon commented Dec 17, 2017

https://github.com/openjournals/joss-papers/blob/joss.00512/joss.00512/10.21105.joss.00512.pdf

@nhejazi
Copy link

nhejazi commented Jan 20, 2018

@wrathematics -- Thanks for submitting your review. The issue with the DOI pointing to the incorrect package version has been fixed by minting a new DOI via Zenodo (for the same GitHub release that we originally opened this review for). Here is a link to the new DOI: https://doi.org/10.5281/zenodo.1155901

Please let us know if there are any other corrections that we can make to complete this review process.

@wrathematics
Copy link
Member

Great, thanks. With the updated DOI, this looks good to me.

This should be good to go as far as JOSS is concerned, but I have a few additional comments that I hope you'll find helpful. The package looks very useful, but the examples don't quite feel right to me. I'd sort of compare it to the idea of something being "pythonic" in python. As you probably know, there are basically two distinct styles in R, base and tidy. This package isn't in the tidy philosophy (which is fine), but it doesn't feel quite right with base either. If we take your linear models example, you could swap out the use of strings for formulas, so the first few lines might look like:

  f <- formula(reg_form)
  mf <- model.frame(f, data=data)
  out_var_ind <- 1

  # split up data into training and validation sets
  train_data <- training(mf)
  valid_data <- validation(mf)

This way you could actually automate all of this for the user, and then the cv_lm() function would only need to do the fitting. Then in cross_validate(), you would pass reg_form=mpg ~ .. You could also play the game of attaching the data to the folds object (sort of like how lm() itself does) to simplify argument passing. You could even do this with a separate formula interface, and leave the current more general interface as is.

There may be some issues here that I haven't considered and you already have, making this harder than my gut instinct is leading me to believe. But if not, I think it's worth considering touching up the interface to use formulas.

@nhejazi
Copy link

nhejazi commented Jan 24, 2018

Perfect, thanks for confirming completion of the JOSS review.

Great to hear that you see the package as useful, and we're glad to hear the critiques/suggestions about the structure of the package, as we'd, of course, like to make sure that potential users feel the code to be natural. I've gone ahead and opened an issue with your comments here -- feel free to add to it at any time. I think it definitely makes sense to update the examples (cv_lm and the like) to match your suggestions, for example, with passing in a model formula rather than a string. In regard to a few of the other suggestions, I do think some of the ideas might clash with the design philosophy of origami, as the goal of the package is not to provide user-friendly functions like cv_lm but the rather the tools for interested users to build such functions (that is, users should really only be relying on functions like training, validation, and cross_validate to essentially write a cross-validation wrapper around practically any process). We'll likely have to think carefully about re-structuring aspects of the core functions to ensure that everything works smoothly with these refinements.

@nhejazi
Copy link

nhejazi commented Jan 29, 2018

@karthik -- I believe we've now completed the review process, but I'm unsure of what the next steps might be in closing this review and generating the approved paper, etc. @wrathematics -- please feel free to correct me if I'm missing something here. Thank you all for your work in this review process.

@karthik
Copy link
Contributor

karthik commented Jan 29, 2018

@nhejazi Great! Can you please update the thread with a DOI (software archive on Zenodo) and I can proceed with the acceptance?

@nhejazi
Copy link

nhejazi commented Jan 30, 2018

Excellent -- we've archived v0.8.0 of the origami package and generated a DOI via Zenodo: DOI: 10.5281/zenodo.1155901. Please let me know if there's anything more we can do on our end.

@arfon arfon added the accepted label Jan 30, 2018
@arfon
Copy link
Member

arfon commented Jan 30, 2018

@whedon set 10.5281/zenodo.1155901 as archive

@whedon
Copy link
Author

whedon commented Jan 30, 2018

OK. 10.5281/zenodo.1155901 is the archive.

@arfon
Copy link
Member

arfon commented Jan 30, 2018

@wrathematics many thanks for your review here and to @karthik for editing this submission.

@nhejazi @jeremyrcoyle - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00512 ⚡️ 🚀 💥

@arfon arfon closed this as completed Jan 30, 2018
@whedon
Copy link
Author

whedon commented Jan 30, 2018

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

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

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

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 volunteering to review for us sometime in the future. You can add your name to the reviewer list here: http://joss.theoj.org/reviewer-signup.html

@nhejazi
Copy link

nhejazi commented Jan 30, 2018

Thank you @wrathematics for the helpful and detailed review, and thank you @karthik for editing the submission for our package. Thanks @arfon for coordinating this process.

@whedon whedon added published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. labels Mar 2, 2020
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