search

openjournals / joss-reviews

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

[REVIEW]: BoltzMM: an R package for maximum pseudolikelihood estimation of fully-visible Boltzmann machines #1193

Closed whedon closed 5 years ago

whedon commented 5 years ago

Submitting author: @andrewthomasjones (Andrew Jones) Repository: https://github.com/andrewthomasjones/BoltzMM Version: 0.1.3 Editor: @yochannah Reviewer: @ja-thomas, @schalkdaniel Archive: 10.5281/zenodo.2563411

Status

status

Status badge code:

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

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

@ja-thomas & @schalkdaniel, 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 @yochannah know.

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

Review checklist for @ja-thomas

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @schalkdaniel

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. @ja-thomas, 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:

ja-thomas commented 5 years ago

Major comments:

Minor comments:

hiendn commented 5 years ago

Hi @ja-thomas, thank you for taking the time to review our paper! We very much appreciate your time and effort! 😊

I will reply to some of your comments now, and @andrewthomasjones will reply to the others at a later stage.

The second author of the paper "Jessica J. Bagnall" does not seem to have commits to the repository. It might be a good idea to specify the contributions of every author in the README.

Unfortunately, Jessica is not actively on GitHub (we will persuade her, yet!) and has therefore made most of her contributions via commits by @andrewthomasjones and I. She has made numerous contributions to the paper via the documentation of the package, the senate data set that she had supplied and analysed from her paper https://hal.archives-ouvertes.fr/hal-01927188v1. As well as with identifying and helping to fix various bugs that were in previous versions of the code.

We have now included an Authorship statement section in the README in order to highlight the contributions of each of the three co-authors of the package.

knn.impute produces warnings in the example

Unfortunately, this cannot be help. No matter how the data is processed or preprocessed before conducting imputation, NAs will be created since there is presence of missingness. Thus, a warning MUST be signalled at some part in time in the workflow. Since it is a little bit unsightly to have warning messages in the README, we have opted to use a suppressWarnings() command in order to make the output more pleasing to the eye.

A reference to the paper given the theoretical framework for this method would be nice. (Or the JOSS paper, since it contains/references the required backround).

This was a great idea! We have now included a Technical references section in the README to address this comment.

There is no citation information in the README. At least the same information as citation("BoltzMM") should be given in the README

Again, great idea! We have now included a Reference to package section in the README that contains exactly the output of the command citation("BoltzMM") as you have suggested.

Thanks again for your time, @ja-thomas. It is very much appreciated.

ja-thomas commented 5 years ago

great, thanks for the quick reply and fix.

I guess the only point open from my side are the issues with the tests discussed here https://github.com/andrewthomasjones/BoltzMM/issues/3. When this is resolved everything is fine from my side.

ja-thomas commented 5 years ago

all done from my side :+1:

yochannah commented 5 years ago

Okay, looking good! @ja-thomas and @schalkdaniel thanks for reviewing. 😄

@schalkdaniel - You still have a couple of checkboxes unchecked. When you get a moment could you let us know if your concerns are resolved, or if not what needs to be done? 👍

schalkdaniel commented 5 years ago

All done from my side too. Everything looks great. 👍

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

yochannah 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.1002/widm.1198 may be missing for title: An introduction to MM algorithms for machine learning and statistical estimation
- https://doi.org/10.1198/0003130042836 may be missing for title: A tutorial on MM algorithms
- https://doi.org/10.1109/tnnls.2015.2425898 may be missing for title: Asymptotic normality of the maximum pseudolikelihood estimator for fully visible Boltzmann machines
- https://doi.org/10.1162/neco_a_00813 may be missing for title: A block successive lower-bound maximization algorithm for the maximum pseudolikelihood estimation of fully visible Boltzmann machines
- https://doi.org/10.1090/conm/080/999014 may be missing for title: Composite likelihood methods
- https://doi.org/10.1055/a-0732-9020 may be missing for title: Pseudolikelihood estimation: some examples
- https://doi.org/10.2307/2346482 may be missing for title: The analysis of multivariate binary data
- https://doi.org/10.1162/neco.2006.18.10.2283 may be missing for title: Consistency of pseudolikelihood estimation of fully visible Boltzmann machines
- https://doi.org/10.1162/neco.2008.04-07-510 may be missing for title: Representation power of restricted Boltzmann machines and deep belief networks
- https://doi.org/10.1016/s0364-0213(85)80012-4 may be missing for title: A learning algorithm for Boltzmann machines

INVALID DOIs

- None
yochannah commented 5 years ago

hmm, this is strange - these DOIs all seem to resolve correctly when I click on them.

I note there is a space as the last character for some of the DOIs which maybe we should fix, but that's only present on three so presumably can't be affecting them all!

@andrewthomasjones - it might be a good idea to remove those spaces from https://github.com/andrewthomasjones/BoltzMM/blob/master/paper.bib just in case?

@arfon / @openjournals/joss-eics - any idea what has whedon grumpy - something I missed or misunderstood maybe?

hiendn commented 5 years ago

Hi @yochannah, spaces have now been removed. I also clicked on all of the links and they all seem to correctly direct to the papers. So 🤷‍♂️? Thank you for you editorial work! 🙇

arfon commented 5 years ago

@yochannah - I've tried fixing the whitespace issue in https://github.com/openjournals/whedon-api/commit/c1d4e5e5c0bdb343207bf19e2dee2e2e8bd7304b.

arfon commented 5 years ago

More generally though, I think the issue is these should be doi fields, not urls, i.e. https://github.com/andrewthomasjones/BoltzMM/blob/master/paper.bib#L28 should be:

doi = {https://doi.org/10.1007/978-1-4614-6868-4}
hiendn 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:

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

hiendn commented 5 years ago

Hi @arfon and @yochannah, I've changed everything that should be a DOI from URL to DOI. There are a couple of items left that do not have DOIs, so I left them as URL. I hope that this resolves the issue! 😄

arfon commented 5 years ago

@whedon check references

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

OK DOIs

- http://doi.org/https://doi.org/10.1002/widm.1198 is OK
- http://doi.org/https://doi.org/10.1198/0003130042836 is OK
- http://doi.org/https://doi.org/10.1109/TNNLS.2015.2425898 is OK
- http://doi.org/https://doi.org/10.1162/NECO_a_00813 is OK
- http://doi.org/http://dx.doi.org/10.1090/conm/080/999014 is OK
- http://doi.org/https://doi.org/10.1162/neco.2006.18.10.2283 is OK
- http://doi.org/https://doi.org/10.1162/neco.2008.04-07-510 is OK
- http://doi.org/https://doi.org/10.1016/S0364-0213(85)80012-4 is OK

MISSING DOIs

- https://doi.org/10.1055/a-0732-9020 may be missing for title: Pseudolikelihood estimation: some examples
- https://doi.org/10.2307/2346482 may be missing for title: The analysis of multivariate binary data

INVALID DOIs

- None
hiendn commented 5 years ago

@whedon check references

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

OK DOIs

- http://doi.org/https://doi.org/10.1002/widm.1198 is OK
- http://doi.org/https://doi.org/10.1198/0003130042836 is OK
- http://doi.org/https://doi.org/10.1109/TNNLS.2015.2425898 is OK
- http://doi.org/https://doi.org/10.1162/NECO_a_00813 is OK
- http://doi.org/http://dx.doi.org/10.1090/conm/080/999014 is OK
- http://doi.org/https://doi.org/10.2307/2346482 is OK
- http://doi.org/https://doi.org/10.1162/neco.2006.18.10.2283 is OK
- http://doi.org/https://doi.org/10.1162/neco.2008.04-07-510 is OK
- http://doi.org/https://doi.org/10.1016/S0364-0213(85)80012-4 is OK

MISSING DOIs

- https://doi.org/10.1055/a-0732-9020 may be missing for title: Pseudolikelihood estimation: some examples

INVALID DOIs

- None
hiendn commented 5 years ago

@arfon , it doesn't look like a DOI exists anywhere on the internet for that last paper, unfortunately. 😞

arfon commented 5 years ago

@arfon , it doesn't look like a DOI exists anywhere on the internet for that last paper, unfortunately.

That's OK. These are only meant to be suggestions - it's fine to have references without DOIs if there isn't one.

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

yochannah commented 5 years ago

@arfon thanks for clarifying what the issue was and @hiendn thank you for resolving the issues! :+1:

yochannah commented 5 years ago

@hiendn @andrewthomasjones - 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? It's probably worth giving the manuscript a final proofread if you feel you need to, as well. 👍

hiendn commented 5 years ago

Hi @yochannah! Thank you for handling the editorial process. the Zenodo DOI/link is http://doi.org/10.5281/zenodo.2563411. And I have given the paper a final proof read and it is all good to go! 😄

labarba commented 5 years ago

👋 @hiendn — Could you edit the title in Zenodo so it matches the title of the JOSS paper? Thanks!

labarba commented 5 years ago

@whedon set10.5281/zenodo.2563411 as archive

labarba commented 5 years ago

@whedon set 10.5281/zenodo.2563411 as archive

whedon commented 5 years ago

OK. 10.5281/zenodo.2563411 is the archive.

hiendn commented 5 years ago

Hi @labarba, the Zenodo archive name has been changed to match the paper title 😄.

labarba commented 5 years ago

@whedon accept

whedon commented 5 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 5 years ago 

OK DOIs

- http://doi.org/https://doi.org/10.1002/widm.1198 is OK
- http://doi.org/https://doi.org/10.1198/0003130042836 is OK
- http://doi.org/https://doi.org/10.1109/TNNLS.2015.2425898 is OK
- http://doi.org/https://doi.org/10.1162/NECO_a_00813 is OK
- http://doi.org/http://dx.doi.org/10.1090/conm/080/999014 is OK
- http://doi.org/https://doi.org/10.2307/2346482 is OK
- http://doi.org/https://doi.org/10.1162/neco.2006.18.10.2283 is OK
- http://doi.org/https://doi.org/10.1162/neco.2008.04-07-510 is OK
- http://doi.org/https://doi.org/10.1016/S0364-0213(85)80012-4 is OK

MISSING DOIs

- https://doi.org/10.1055/a-0732-9020 may be missing for title: Pseudolikelihood estimation: some examples

INVALID DOIs

- None
whedon commented 5 years ago

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

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/491, 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 commented 5 years ago

@hiendn — I'm going to have a few more editorial fixes to request.

¶1 that is able densely represent >> is able to when d is large and sample size n >> what about the sample size? (missing a verb and parallel construction)

In general, I find the whole first page really jargon-heavy and I wonder if you would consider editing this to be more readable. E.g., what is "latent variable construction"? Do you mean the same by "complex form" and "elaborate form" And what do you mean by that? Jus that it's hard to implement? Or is it really "intractable"? Because then you talk about a "tractable simplification" ... so many big words. Are they all necessary for the JOSS audience to plough through, to finally learn what the software does on page 2?

Many of your DOIs are not resolving because you have a full URL instead of the DOI in the BibTeX field, so the link appears broken. Please check. [EDIT] To clarify, the links come out as https://doi.org/https://doi.org/10.1016/S0364-0213(85)80012-4 which don't resolve.