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]: fmcmc: A friendly MCMC framework #1427

Closed
whedon opened this issue May 3, 2019 · 34 comments

Comments

Projects
None yet
7 participants
@whedon
Copy link
Collaborator

commented May 3, 2019

Submitting author: @gvegayon (George Vega Yon)
Repository: https://github.com/USCbiostats/fmcmc
Version: 0.2-0
Editor: @arokem
Reviewer: @fabian-s
Archive: 10.5281/zenodo.3272759

Status

status

Status badge code:

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

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

@fabian-s, 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 @arokem know.

Please try and complete your review in the next two weeks

Review checklist for @fabian-s

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: 0.2-0
  • Authorship: Has the submitting author (@gvegayon) 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

This comment has been minimized.

Copy link
Collaborator Author

commented May 3, 2019

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @fabian-s 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

This comment has been minimized.

Copy link
Collaborator Author

commented May 3, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented May 3, 2019

@fabian-s

This comment has been minimized.

Copy link

commented May 10, 2019

LICENSE is a placeholder: USCbiostats/fmcmc#5

tagged release version is missing: USCbiostats/fmcmc#6

I don't see a clear "statement of need and audience" in the repo.

Documentation issue for proposal: USCbiostats/fmcmc#7

Another "documentation" issue I can't pin on any specific function is the more general question on how to specify any priors? MCMC only takes a "log-likelihood" as its main argument, your examples do not seem to use any. That seems very weird and impractical to me, but possibly I just misunderstand...?

DOIs are missing in references: USCbiostats/fmcmc#8

You have a "statement of need" in your paper which I find rather hyperbolic, TBH. The ultimate flexibility you claim comes at the cost of users being required to 1) implement the (log-)likelihood/posterior themselves, and 2) figuring out a suitable scaling of the proposal (kernel) for reasonable accceptance rates, which will be challenging in almost any interesting application. In addition, it seems like a stretch to claim a "pure R" implementation if some of the heavy lifting is actually being done in C++. Could the last paragraph be phrased more accurately?

Typos: "this alows" --> allows, "convergance" --> convergence, "existing mcmc packages" --> MCMC packages

@gvegayon

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2019

Thanks @fabian-s! We will go through the issues and let you know asap.

@gvegayon

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2019

Dear @fabian-s,

I've addressed most of your comments on USCbiostats/fmcmc#9 (thanks for them!). Regarding the rest of the comments:

Another "documentation" issue I can't pin on any specific function is the more general question on how to specify any priors? MCMC only takes a "log-likelihood" as its main argument, your examples do not seem to use any. That seems very weird and impractical to me, but possibly I just misunderstand...?

There is no need to have a special argument for that. Any set of particular priors can be added directly in the objective function. For example, suppose that your log-likelihood function looks like this:

ll <- function(p, X., y.) {
  
  sum(dnorm(y. - (p[1] + X.*p[2]), sd = p[3], log = TRUE))

}

If you want to add priors to the third parameter, say you think it is distributed Beta with parameters 2 and 8, you can do it by setting:

ll <- function(p, X., y.) {
  
  sum(dnorm(y. - (p[1] + X.*p[2]), sd = p[3], log = TRUE)) +
    dbeta(p[3], 2, 8, log = TRUE)

}

So there is no need to do anything else than that. The algorithm will just work.

You have a "statement of need" in your paper which I find rather hyperbolic, TBH. The ultimate flexibility you claim comes at the cost of users being required to 1) implement the (log-)likelihood/posterior themselves, and 2) figuring out a suitable scaling of the proposal (kernel) for reasonable accceptance rates, which will be challenging in almost any interesting application

I've added a statement of need as an introduction in the README.md file in which I explicitly state who should be using this R package. This, as other MCMC packages, are for those who want to implement their own model via a log-likelihood function (for example). In particular:

  1. Yes, that is exactly what you are supposed to do in this type of packages:

  2. That is the idea of the personalizable transition kernel, the users can add that by themselves if they need to (also, will probably add a kernel with automatic adjustment in the future, thanks for the idea :)!)

In addition, it seems like a stretch to claim a "pure R" implementation if some of the heavy lifting is actually being done in C++. Could the last paragraph be phrased more accurately?

That was a very good point. We actually figured out how to rewrite a function (which was in C++) to be as efficient using only base R. Right now the package doesn't need to be compiled anymore.

Please let me know if you have any additional comments :).

@labarba

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

👋 @fabian-s — looks like the author is waiting here for your reply?

@arokem

This comment has been minimized.

Copy link

commented Jun 24, 2019

@fabian-s : have you had a chance to take another look?

@fabian-s

This comment has been minimized.

Copy link

commented Jun 24, 2019

Sorry for the delay, I was travelling. Seems fine now. Mayybe add a DOI to the JSS R2WinBUGS paper as well?

@gvegayon

This comment has been minimized.

Copy link
Collaborator

commented Jun 24, 2019

Hey

Sorry for the delay, I was travelling. Seems fine now. Mayybe add a DOI to the JSS R2WinBUGS paper as well?

Done, and added another MCMC R package/paper that was relevant for the literature review. @fabian-s I will close the issues you started. Thanks!

@gvegayon

This comment has been minimized.

Copy link
Collaborator

commented Jun 24, 2019

Also, the travis-ci build is failing in Mac due to an issue regarding LaTeX (which I cannot control). Windows and Ubuntu is building OK.

@arokem

This comment has been minimized.

Copy link

commented Jun 27, 2019

@fabian-s : are all your comments addressed? I see that the "statement of need" box is still unchecked.

@gvegayon : what is the issue with the CI build? Is this something related to the JOSS paper document?

gvegayon added a commit to USCbiostats/fmcmc that referenced this issue Jun 28, 2019

@gvegayon

This comment has been minimized.

Copy link
Collaborator

commented Jun 28, 2019

@gvegayon : what is the issue with the CI build? Is this something related to the JOSS paper document?

No, it was something related to the version of OSX I was using. Using a more recent one fixes the issue (confirmed here).

@fabian-s I've added you to the list of authors of the R package with the role "rev" (Reviewer). LMK if that's OK with you (pulled your ORCID from one of your papers).

@fabian-s

This comment has been minimized.

Copy link

commented Jun 29, 2019

all is well. forgot to check the box.

@arokem

This comment has been minimized.

Copy link

commented Jul 7, 2019

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2019

@arokem

This comment has been minimized.

Copy link

commented Jul 7, 2019

@whedon check references

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2019

Attempting to check references...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2019


OK DOIs

- 10.2307/2334940 is OK
- 10.1063/1.1699114 is OK
- 10.18637/jss.v042.i09 is OK
- 10.18637/jss.v076.i01 is OK
- 10.1214/ss/1177011136 is OK
- 10.18637/jss.v012.i03 is OK

MISSING DOIs

- None

INVALID DOIs

- None
@arokem

This comment has been minimized.

Copy link

commented Jul 7, 2019

@gvegayon : looks like all is well here. I have one small comment on the manuscript. The word "Gaussian" is derived from a name and should be capitalized (appears twice, as far as I can see).

Once you correct this, could you please create an archive of the software (e.g., using https://zenodo.org) and post the DOI on this thread?

gvegayon added a commit to USCbiostats/fmcmc that referenced this issue Jul 8, 2019

gvegayon added a commit to USCbiostats/fmcmc that referenced this issue Jul 8, 2019

@gvegayon

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

Done! Here is the DOI 10.5281/zenodo.3272759 Thanks!

*edit: Added link to DOI

@gvegayon

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

Done! Here is the DOI 10.5281/zenodo.3272759 Thanks!

And the new version is 0.2-0

@arokem

This comment has been minimized.

Copy link

commented Jul 8, 2019

@whedon set 10.5281/zenodo.3272759 as archive

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 8, 2019

OK. 10.5281/zenodo.3272759 is the archive.

@arokem

This comment has been minimized.

Copy link

commented Jul 8, 2019

@whedon set 0.2-0 as version

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 8, 2019

OK. 0.2-0 is the version.

@arokem

This comment has been minimized.

Copy link

commented Jul 8, 2019

@openjournals/joss-eics : I believe this one is ready to 🚢

@gvegayon

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

@openjournals/joss-eics : I believe this one is ready to

Just in case, I do have an issue with my last name, I have two of them! "Vega Yon" and not "Yon" only (see this issue)

@arokem

This comment has been minimized.

Copy link

commented Jul 8, 2019

Oh - sorry about that! Maybe this USCbiostats/fmcmc#10 will help?

@gvegayon

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

I don't think it works. See here.

@kyleniemeyer

This comment has been minimized.

Copy link

commented Jul 8, 2019

I think @arfon needs to do this manually, unfortunately.

@arfon arfon added the accepted label Jul 9, 2019

@arfon

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

I think @arfon needs to do this manually, unfortunately.

Done.

@fabian-s - many thanks for your review here and to @arokem for editing this submission

@gvegayon - your paper is now accepted into JOSS ⚡️🚀💥

@arfon arfon closed this Jul 9, 2019

@whedon

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 9, 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](http://joss.theoj.org/papers/10.21105/joss.01427/status.svg)](https://doi.org/10.21105/joss.01427)

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

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

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.