[REVIEW]: KLRfome - Kernel Logistic Regression on Focal Mean Embeddings

whedon commented 6 years ago

Submitting author: @mrecos (Matthew Harris) Repository: https://github.com/mrecos/klrfome Version: v2.2.0 Editor: @arfon Reviewer: @benmarwick Archive: 10.5281/zenodo.2598673

Status

Status badge code:

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

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

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

Make sure you're logged in to your GitHub account
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 @leeper know.

Review checklist for @benmarwick

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: v2.2.0
[x] Authorship: Has the submitting author (@mrecos) made major contributions to the software? Does the full list of paper authors seem appropriate and complete? BM: I think Szabo should be noted in the readme as someone who made a contribution that made this pkg possible BM: done

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] 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

The package includes all the following forms of documentation:

[x] A statement of need clearly stating problems the software is designed to solve and its target audience in README BM: very nicely stated.
[x] Installation instructions: for the development version of package and any non-standard dependencies in README
[x] Vignette(s) demonstrating major functionality that runs successfully locally BM: There is no vignette, but the README gives a very thorough walk-through. I think I've seen a method to have a vignette, and auto-extract it into the README on pkg build, that would help to fulfil this requirement. BM: done
[x] Function Documentation: for all exported functions in R help BM: documentation is present, but uneven. Descriptions and examples are missing for many functions. These should be added. Spelling: scaler should be scalar. The KLR function has a bit of commented-out code in it that probably should be removed. BM: done
[x] Examples for all exported functions in R Help that run successfully locally BM: not yet, though the readme is a good start. BM: done
[x] Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R). BM: There is no 'Maintainer' field in description. The pkg referred to in the CONTRIBUTING is DistRegLMERR, presumably an earlier name for this pkg, but should be changed. BM: done

Software paper

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

Additional criteria (from rOpenSci)

BM: I pasted these over from the rOpenSci onboarding

[x] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines: https://github.com/ropensci/onboarding/blob/master/packaging_guide.md BM: not yet, still some documentation to add. There is no NEWS file. There are no tests. BM: done
- [x] Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine. BM: not yet. BM: done
Final approval (post-review)
[x] The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 3

Review Comments

The author has done an excellent job of fulfilling the requirements for JOSS. In particular, it's an excellent example of superb package documentation to the point that I believe any undergraduate archaeology student (or related field) should be able to make sense of what is the need and purpose, and quickly start using the pkg. The vignette is outstanding and deserves a wide audience. Well done!

whedon commented 6 years ago

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @benmarwick 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:

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

watching

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 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

leeper commented 6 years ago

@benmarwick I can see you've made some progress on this review. Can you open issues over at the source repo for any issues you'd like to see addressed? Thanks!

benmarwick commented 6 years ago

Righto, thanks for the tip, I've opened a few relevant issues now.

leeper commented 6 years ago

@benmarwick Great. Thanks! Let us know if you have any other comments at this point.

@mrecos Please try to address the issues opened by @benmarwick. The matters of examples and tests are particularly important.

mrecos commented 6 years ago

Great! Thanks @benmarwick and @leeper. I will address in short order and report back.

leeper commented 6 years ago

@mrecos Just wanted to check in. Are you still planning to make revisions for this?

mrecos commented 6 years ago

Hello @leeper , yes I am. My apologies. I will work on it this coming week.

leeper commented 6 years ago

@mrecos Just another nudge on this.

mrecos commented 6 years ago

Thanks @leeper - Making edits/additions/commits now.

arfon commented 6 years ago

@mrecos - how are you getting along here?

arfon commented 6 years ago

@mrecos - did you manage to make your revisions?

arfon commented 6 years ago

Just heard back from the author (via email) that he's still interested in pursuing this publication with JOSS and is working on changes as a result of the review.

arfon commented 6 years ago

@whedon assign @arfon as editor

mrecos commented 5 years ago

Hello @afron. Quick update: I took care of 2 of 3 issues on my repo. TODO - convert existing how-to to vignette and unit tests.

arfon commented 5 years ago

:wave: @mrecos - how are you getting along here?

mrecos commented 5 years ago

Hello @arfon . Going well. I took care of all points except for the unit tests. I am currently implementing those.

mrecos commented 5 years ago

Hello @arfon . I have completed the code updates requested by Ben via his review and Issues. These include general code cleanup, documentation of all public functions, a vignette, and unit testing. Please review and advise. Thanks!

arfon commented 5 years ago

Hello @arfon . I have completed the code updates requested by Ben via his review and Issues. These include general code cleanup, documentation of all public functions, a vignette, and unit testing. Please review and advise. Thanks!

Great!

@benmarwick - when you get a chance, could you please come and take another look?

labarba commented 5 years ago

👋 @benmarwick — It looks like we are waiting for you to check the author's responses to your comments. What's your status?

benmarwick commented 5 years ago

Thanks for the reminder. I've had another look at the package and everything looks great! The author has done an excellent job of fulfilling the requirements for JOSS. In particular, it's an excellent example of superb package documentation to the point that I believe any undergraduate archaeology student (or related field) should be able to make sense of what is the need and purpose, and quickly start using the pkg. The vignette is outstanding and deserves a wide audience. Well done!

labarba commented 5 years ago

I see some unchecked items in the checklist—if you're satisfied, can you tick off those items?

benmarwick commented 5 years ago

Done!

arfon commented 5 years ago

@whedon accept

whedon commented 5 years ago

No archive DOI set. Exiting...

arfon commented 5 years ago

@mrecos - At this point could you make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission.

mrecos commented 5 years ago

Hello @arfon - I create a the "JOSS Release" and committed to Zenodo for a new DOI (10.5281/zenodo.2598673). I checked the DOI badge on the KLRfome repo and all seems well. Let me know if this covers it. Thanks!

labarba commented 5 years ago

@whedon set 10.5281/zenodo.2598673 as archive

whedon commented 5 years ago

OK. 10.5281/zenodo.2598673 is the archive.

labarba 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:

labarba commented 5 years ago

Can you edit the metadata of the Zenodo deposit (no need to get new version or new DOI) so the title and author list match the JOSS paper?

labarba commented 5 years ago

@whedon set v2.2.0 as version

whedon commented 5 years ago

OK. v2.2.0 is the version.

mrecos commented 5 years ago

Hello @labarba, ok updated! I had found a typo in the release version of the JOSS paper markdown just now. I pushed the edits to the repo, but they are now post 2.2.0 release. Should I do redo the release? https://zenodo.org/record/2598673#.XJE_3FVKipo

labarba commented 5 years ago

If it's just an edit on the JOSS paper, I don't see a need to redo the release (the final JOSS paper is archived on its own in the journal).

mrecos commented 5 years ago

ok, then we should be good to go. Please let me know how else I can help.

labarba commented 5 years ago

@whedon accept

whedon commented 5 years ago

Attempting dry run of processing paper acceptance...

whedon commented 5 years ago

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/561

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

@whedon accept deposit=true

whedon commented 5 years ago


OK DOIs

- None

MISSING DOIs

- https://doi.org/10.1198/106186005x25619 may be missing for title: Kernel logistic regression and the import-vector machine
- https://doi.org/10.1561/2200000060 may be missing for title: Kernel Mean Embedding of Distributions: A Review and Beyond
- https://doi.org/10.1109/msp.2013.2252713 may be missing for title:  Kernel Embeddings of Conditional Distributions: A Unified Kernel Framework for Nonparametric Inference in Graphical Models

INVALID DOIs

- None

labarba commented 5 years ago

Please check if the suggested "Mising DOIs" are indeed missing, or are flukes of whedon's Crossref query.

labarba commented 5 years ago

@arfon — Are you OK with the final version of the paper?

@mrecos — This is also your final chance to check the proof, and correct any typos / grammar / punctuation / author name spelling / reference titles needing cap protection ...

mrecos commented 5 years ago

@labarba whedon was correct, those citations had DOIs to include. I commit/pushed a new paper.bib with the DOIs included. Aside from that, the proof looks good!

arfon 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:

mrecos commented 5 years ago

@whedon accept deposit=true

openjournals / joss-reviews