-
Notifications
You must be signed in to change notification settings - Fork 1
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]: Pigeons.jl: Distributed sampling from intractable distributions #139
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @nsailor, @georgebisbas it looks like you're currently assigned to review this paper 🎉. Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post. ⭐ Important ⭐ If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/JuliaCon/proceedings-review) 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:
For a list of things I can do to help you, just type:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
Failed to discover a |
Wordcount for |
|
Failed to discover a valid open source license. |
|
👋 @georgebisbas, please update us on how your review is going (this is an automated reminder). |
👋 @nsailor, please update us on how your review is going (this is an automated reminder). |
This work is an implementation of the Parallel Tempering method in Julia, an improvement over MCMC for obtaining samples form otherwise difficult to sample probability distributions. I greatly appreciate the attention given to what the authors term "strong parallelism invariance" (SPI), roughly meaning that the result should not depend on the degree of parallelism (especially considering the complexities of finite-precision floating-point arithmetic). This package also builds upon a Julia translation of a Java library called Splittable Random previously published by the authors, allowing deterministic random number generation across multiple threads. Overall, I think this work is a great contribution to the Julia ecosystem, notwithstanding the following minor points:
In writing the above, it is possible that I have misunderstood some part of this work, in which case, please feel free to correct me. |
Thank you for your comments! We will work on updating the manuscript, package, and documentation to account for your feedback. |
Dear @pitsianis -- we were wondering what the expected timeline for the reviews is? Should we perhaps just respond to @nsailor and not wait for @georgebisbas, or should we wait until we have both sets of comments? Thank you. |
Please answer any outstanding issues; you don't need to do this sequentially. This exchange will also remind @georgebisbas to wrap up his review. |
Thanks for the clarification. |
Dear @miguelbiron and @pitsianis, my apologies for the delay here. |
No worries @georgebisbas, thank you for prioritizing |
Hi again, apologies for the delay, I have partly written a draft, aiming to complete my review in the next days. |
First, I would like to thank the authors for their patience over the last few months. Pigeons.jl offers a high-level API to leverage shared- and distributed-memory parallelism via Julia's built-in multi-threading features and MPI.jl, a Julia wrapper for MPI. Strengths :
Weaknesses/Questions:
Minor:
|
Thank you to both reviewers for their comments! Now that all reviews are in, we will get back to you shortly with updates and responses to the questions raised. |
We thank the reviewers for their insightful comments! Our responses to each reviewer are presented below. Reviewer 1 (Jasan Barmparesos @nsailor)
Additionally, the paper mentions Pigeons being able to run on "thousands of MPI-communicating machines". It may be worth clarifying if this is something that has been tested or a possibility given the package's design.
I was not able to get a speedup with the toy MVN example using multiple threads (see this issue in the project repository). In general, it would be very helpful to have some hints in the package's documentation for tuning the degree of parallelism for a given problem, especially if the speedup from parallelism is not linear.
It would be great to have additional examples, as described in this issue
Reviewer 2 (George Bisbas @georgebisbas) What are the memory requirements of a "typical large enough" problem to be tackled?
What are the advantages of DMP versus SMP only?
Why are any strong scaling graphs not included?
After reading the paper, I feel that MPI works correctly, but with no clue on why it is needed and what performance benefits it brings to the table.
Have any of the additional targets been implemented in the meantime? Could the paper be updated there?
References |
My name is now @editorialbot |
@pitsianis Do we need to use the editorial bot for our rebuttal? Just checking that the reviewers (@nsailor, @georgebisbas) have been notified of our responses. Thanks! |
@nsailor & @georgebisbas, please look at the author's answer to your comments. If appropriate, check the remaining boxes in your checklists and/or follow up with any remaining questions and challenges so the review will move ahead. |
Hi @pitsianis , @nikola-sur , I would expect that the editorial bot will create some new pdf. Is that right? |
@editorialbot generate pdf |
@whedon generate pdf |
My name is now @editorialbot |
@editorialbot commands |
Hello @nikola-sur, here are the things you can ask me to do:
|
@editorialbot generate pdf |
1 similar comment
@georgebisbas The new PDF can be found above. |
Hi @nikola-sur , thanks for updating the manuscript. I have updated my checklist above accordingly. I am more than happy with the response and the edits. I think my only remaining checkboxes are:
You may need to have a look here: https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/setting-guidelines-for-repository-contributors
I did not see DOIs in the software as far as I recall. I think we are nearly there. |
Hi @nikola-sur , thank you for responding to the review points, I think they have been addressed sufficiently. I don't have any other comment except for the two checklist items mentioned by @georgebisbas above. Best wishes, |
Thank you both @nsailor @georgebisbas . Just to be clear on the contribution guidelines issue: this is referring to the repo, correct? Like having a proper CONTRIBUTING.md, for example. Not the paper itself, right? |
@miguelbiron , yes. |
Thank you @georgebisbas . Then Julia-Tempering/Pigeons.jl#289 should cover it. And we have added dois to the reference to the best of our abilities. There are 2 conference papers without known dois. I'm generating a new version now. |
@editorialbot generate pdf |
Ah no wait a second. I forgot to commit stuff... |
@editorialbot generate pdf |
Thank you for that @miguelbiron . @pitsianis , the checklist is now complete from my side. |
@pitsianis It seems that both reviewers are now satisfied with the changes. Is there anything else that we should do? Please let us know! |
Indeed very nice work and paper. Unrelated, it would be nice to have |
@editorialbot check references |
|
Post-Review Checklist for Editor and AuthorsAdditional Author Tasks After Review is Complete
Editor Tasks Prior to Acceptance
|
@nikola-sur please have another look at the DOIs of your references, see the report above. |
Submitting author: @nikola-sur (Nikola Surjanovic)
Repository: https://github.com/Julia-Tempering/Pigeons-Paper
Branch with paper.md (empty if default branch):
Version:
Editor: @pitsianis
Reviewers: @nsailor, @georgebisbas
Archive: Pending
Status
Status badge code:
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
@nsailor & @georgebisbas, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @pitsianis know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Review checklist for @nsailor
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Paper format
paper.tex
file include a list of authors with their affiliations?Content
Review checklist for @georgebisbas
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Paper format
paper.tex
file include a list of authors with their affiliations?Content
The text was updated successfully, but these errors were encountered: