-
-
Notifications
You must be signed in to change notification settings - Fork 38
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]: Spiner: Performance Portable Routines for Generic, Tabulated, Multi-Dimensional Data #4367
Comments
Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks. 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:
|
|
Wordcount for |
Failed to discover a valid open source license |
Review checklist for @lgarrisonConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
@Yurlungur, @lgarrison, @jzrake — This is the review thread for the paper. All of our communications will happen here from now on. Thanks again for agreeing to participate! Please read the "Reviewer instructions & questions" in the first comment above, and generate your checklists by commenting The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention We aim for the review process to be completed within about 4-6 weeks but please try to make a start ahead of this as JOSS reviews are by their nature iterative and any early feedback you may be able to provide to the author will be very helpful in meeting this schedule. |
Don't worry about this comment from the bot - it expects the code and paper to both be on the same branch, but this is not the case here, and not a problem! |
|
Thanks, @lgarrison, @jzrake , @dfm . Looking forward to interacting with you during the review process. |
@lgarrison, @jzrake — This is just a little ping to make sure that this review stays on your radar. It's good to start chipping away at the checklists sooner rather than later! |
Review checklist for @jzrakeConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
@Yurlungur -- I hope I'm doing this right. Here are two bits of feedback based on my first impression from the PDF writeup.
|
Thanks, @jzrake It'll take me a little bit to gather a formal response for these comments and implement changes to the manuscript. I should have something soon. |
@jzrake thanks for the comments. I just updated the manuscript based on your feedback. The relevant commit is here. Here's a formal little writeup to explain the answers to your questions:
|
@Yurlungur thank you for the thoughtful reply. I hope this content can be included in the PDF writeup, as I think it significantly clarifies the motivation and objectives of the project. @dfm is the PDF writeup intended to be very concise, or can it be as detailed as the reply above? EDIT: I only just saw the additions to the PDF writeup. I'll give another round of comments soon, if I have any. |
@editorialbot generate pdf |
@Yurlungur, @jzrake — Re: manuscript scope. Our goal here is for the manuscript to be brief with the documentation page being the primary standalone source of information, so it's generally a good idea to add some words to the documentation whenever you're extending the manuscript. I prefer the manuscript to not have any unique material (everything should be in the docs one way or another), but that's just my preference and not a requirement! |
Thanks for the clarification, @dfm. Currently there's no statement of need in the docs. I will add one that mirrors the discussion here and in the manuscript. Thanks, @jzrake. Please let me know if the additions to the manuscript address concerns, or if I should further extend it based on the discussion here. |
☝️ documentation updated in linked PR (now merged). |
Hi @Yurlungur! I'm working on my review and as per @dfm's suggestion, I'll ask questions as they come up. On the performance, the results compared to the CPU look impressive, but it still feels like some additional context would be helpful. The paper motivates Spiner by saying the interpolation should not be the limiting factor in a radiation transport simulation—is that now the case? Do you have timings you can share for the interpolation step versus the total time step in a typical simulation? Alternatively, you could estimate the achieved FLOPs compared to theoretical peak, either by using the CUDA profiler or by estimating the number of operations by hand. Either the % of peak or a comparison to the total simulation time would help contextualize your success here! Finally, can you clarify in the writeup whether the performance test was done in single or double precision? Perhaps double, since the paper starts by discussing precision limitations in texture interpolation? |
Thanks @jzrake @lgarrison ! I appreciate the time you spent engaging with me and the project. Thanks @dfm I've merged the PR, done a final readthrough, and incremented version number.
|
@editorialbot generate pdf |
@editorialbot set 10.5281/zenodo.6800124 as archive |
Done! Archive is now 10.5281/zenodo.6800124 |
@editorialbot set v1.5.1 as version |
Done! version is now v1.5.1 |
@editorialbot recommend-accept |
|
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉📄 Download article If the paper PDF and the deposit XML files look good in openjournals/joss-papers#3350, then you can now move forward with accepting the submission by compiling again with the command |
@Yurlungur — I've now handed this off to the managing editors to do the final processing. There may be some final edits or other changes, but the process should be fairly quick. Thanks again for your submission and for your responses to all the suggestions from @lgarrison and @jzrake! @lgarrison, @jzrake — Thanks again for your reviews of this submission. Thanks for the time that you took and the thorough and constructive comments that you made. We couldn't do this without you, and I really appreciate you volunteering your time!! @openjournals/joss-eics — Some notes: (1) @jzrake's checklist does not have the performance box checked because he didn't have the required hardware (see comment: #4367 (comment)), but @lgarrison was able to replicate the results so I'm happy with this as it stands; (2) the bot has gone rogue with it's recommended missing DOI above... I'm not sure what it's thinking! |
Well, the DOI does lead to a work that matches the title, so I understand why this was offered as a possibility... Also, I'm the AEiC this week and will check this now |
Thanks all! |
I've suggested some minor changes in 2 PRs, both of which are already merged |
@editorialbot recommend-accept |
|
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉📄 Download article If the paper PDF and the deposit XML files look good in openjournals/joss-papers#3351, then you can now move forward with accepting the submission by compiling again with the command |
sorry, one more PR: lanl/spiner#44 |
@editorialbot accept |
|
🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦 |
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨 Here's what you must now do:
Any issues? Notify your editorial technical team... |
Congratulations to @Yurlungur (Jonah Miller) and co-authors!! And thanks to @lgarrison and @jzrake for reviewing, and to @dfm for editing! |
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: We need your help! The 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:
|
Submitting author: @Yurlungur (Jonah Miller)
Repository: https://github.com/lanl/spiner
Branch with paper.md (empty if default branch): joss-paper
Version: v1.5.1
Editor: @dfm
Reviewers: @lgarrison, @jzrake
Archive: 10.5281/zenodo.6800124
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
@lgarrison & @jzrake, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review.
First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @dfm 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 ✨
Checklists
📝 Checklist for @lgarrison
📝 Checklist for @jzrake
The text was updated successfully, but these errors were encountered: