Closed editorialbot closed 4 months ago
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:
@editorialbot commands
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdf
Software report:
github.com/AlDanial/cloc v 1.88 T=0.04 s (731.6 files/s, 291412.4 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Julia 12 145 96 702
SVG 3 3 3 335
Markdown 9 49 0 253
TeX 1 11 0 129
Jupyter Notebook 1 0 10368 110
YAML 3 0 2 107
TOML 2 5 0 30
-------------------------------------------------------------------------------
SUM: 31 213 10469 1666
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repository
Wordcount for paper.md
is 2015
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1088/1367-2630/ac4ef2 is OK
- 10.1103/physrevb.100.140401 is OK
- 10.48550/arXiv.2204.10874 is OK
- 10.48550/arXiv.2305.17082 is OK
- 10.48550/arXiv.2305.16964 is OK
- 10.1088/0953-8984/26/10/103202 is OK
- 10.1103/physrevb.83.020410 is OK
- 10.1038/s41567-020-01040-y is OK
- 10.1103/physrevb.58.293 is OK
- 10.1103/physrevlett.76.4250 is OK
- 10.1038/s41567-018-0053-8 is OK
MISSING DOIs
- None
INVALID DOIs
- None
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@Datseris, @mdavezac, @dawbarton Thanks for agreeing to review this submission! This is the review thread for the paper. All of our communications will happen here from now on. :+1:
As you can see above, you each should use the command @editorialbot generate my checklist
to create your review checklist. @editorialbot commands need to be the first thing in a new comment.
As you go over the submission, please check any items that you feel have been satisfied (and if you leave notes on each item that's even better). There are also links to the JOSS reviewer guidelines. I find it particularly helpful to also use the JOSS review criteria and review checklist docs as supplement/guides to the reviewer checklist @editorialbot will make for you.
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, reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#6263
so that a link is created to this Issue thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.
We aim for reviews to be completed within about 4 weeks. Please let me know if either of you require some more time (that's perfectly okay). We can also use @editorialbot to set automatic reminders if you know you'll be away for a known period of time.
Please feel free to ping me (@matthewfeickert) if you have any questions/concerns.
@mekise @Datseris, sorry I missed this during the pre-review, but I now see that you're both employed at the University of Exeter which might be a Conflict of Interest violation. It looks like you work in seperate departments (Physics vs. Mathematics and Statistics), but can you provide context on if you have any overlap at the University?
No overlap at all.
No overlap at all.
@mekise @Datseris okay great. I think we can move forward with the review then with the conflict being recorded and then waived, as having no overlap means you fall into the example of both being "employed by the same very large organization but in different units without any knowledge of each other." :+1:
As all reviewers have their checklist generated now I'll have @editorialbot give us reminders in 3 weeks to follow up on the initial state of the review. (I'll have @dawbarton's be much longer though as they won't be joining the review till March.)
@editorialbot remind @Datseris in 3 weeks
Reminder set for @Datseris in 3 weeks
@editorialbot remind @mdavezac in 3 weeks
Reminder set for @mdavezac in 3 weeks
@editorialbot remind @dawbarton in 8 weeks
Reminder set for @dawbarton in 8 weeks
Hi @matthewfeickert ,
before investing more time in the review, we need to clarify something regarding the scholarly effort point. I read the JOSS guidliness for substantial scholarly effort as I was ticking the relevant checkmark: https://joss.readthedocs.io/en/latest/submitting.html#substantial-scholarly-effort . But then I decided to check the source code to confirm my tick. The source code is much less than 1000 loc, and by my count it just barely reaches 300 (I've excluded documentation in counting source loc) which according to the current guidelines would not be enough to satisfy the requirements.
I have not yet took a detailed look at the paper itself. The repository source code however appears to be a structuring of logical containers for running stochastic differential equations. It sets up the equations and the noise term and then runs DifferentialEquations.jl (the julia package for running SDEs) for the user: https://github.com/quantum-exeter/SpiDy.jl/blob/3c0042b87381707a43dcae39c65eed823dc46808/src/Dynamics.jl#L28 . diffeqsolver
which is the main export of the software is doing what I described above.
Please advise how this relates to the guidelines before we invest more time into the review!
all the best, George
Thanks for raising this @Datseris. I am currently traveling and will laregly be offline until 2024-02-02, so I'll raise this with the Physics and Engineering Editor-in-Chief to give feedback on in a more timely manner.
Hi @Datseris and @matthewfeickert , just wanted to mention that this was already been raised and resolved in pre-review (see https://github.com/openjournals/joss-reviews/issues/5934#issuecomment-1799230068). Hope this helps.
Hi @Datseris @matthewfeickert, yes, we proceeded with review with the understanding that SpectralDensities.jl
was also part of the submission package.
:wave: @Datseris, please update us on how your review is going (this is an automated reminder).
:wave: @mdavezac, please update us on how your review is going (this is an automated reminder).
Hi, I am submitting my review:
I ticked off most of my review checkboxes. However, I do have some standing concerns:
I've raised two issues in the main repo regarding testing and community guidelines. The latter is also an issue with SpectralDensities.jl. Except for these outstanding issues, I believe SpiDy.jl and SpectralDensities.jl satisfy all requirements for publication in JOSS.
Sorry All, I've been sick and totally offline from JOSS for the last week. I will catch up on the activity here in the next day.
:wave: @mekise I just wanted to check in. Do you have a plan in place to address the issues that @Datseris and @mdavezac have opened?
@Datseris, thanks for the summary of the outstanding items that you identified in your review. Can you open up GitHub Issues on https://github.com/quantum-exeter/SpiDy.jl with these points (of the form [JOSS Review] Issue Title
) and link back to this Issue (openjournals/joss-reviews#6263
)?
Also, @dawbarton, while the @editorialbot reminder that I set for you hasn't gone off yet, have you started your review yet (not expecting you to have finished given the timelines we discussed)? If not, if it is feasible to don that in the next week that would be great.
Hi @matthewfeickert , yes we do have plans indeed. We will start taking care of the issues raised and prepare a reply starting next week. Unfortunately, most of us are out for conferences these days. Thank you!
Totally understood @mekise. I'll have @editorialbot set a reminder for us to check in again in April.
@editorialbot remind @mekise in 4 weeks
Reminder set for @mekise in 4 weeks
:wave: @dawbarton, please update us on how your review is going (this is an automated reminder).
:wave: @mekise, please update us on how things are progressing here (this is an automated reminder).
Hi @mdavezac
Thank you for your assessment. We are pleased to see that SpiDy.jl satisfies the requirements for publication.
Hi, I am submitting my review:
I ticked off most of my review checkboxes. However, I do have some standing concerns:
- The tests tickbox I left open as the main repo does not appear to have adequate tests. It is only checking that the ODE Solving yielded a "success" code. But this just means that solutions did not diverge to infinity. There are no checks whatsoever regarding whether the solutions are physically correct or whether they reflect the properties expected by the physical system they emulate. The tests must be made more extensive and test for the accuracy of setting up the problem and the resulting physical solution.
- I think the name of the software can be improved. The software description reads: "Non-Markovian stochastic dynamics for spin and harmonic oscillators. " and the paper summary introduces it as a software to "solve the non-Markovian stochastic dynamics of interacting classical spin vectors and harmonic oscillator networks in contact with a dissipative environment". There is simply no way one can associate the abbreviated name "SpiDy" with this very specific target application. Even "SpinDynamics.jl" appears to not be an accurate name, as the software is described as much more specific than treating any kind of spin dynamics (such as Ising models and quantum statistical mechanics type of problems). I think it is to the benefit of the authors to change the name to something that will help people not familiar yet with the software understand whether this software is for them or not at a first glance.
- The performance discussion in the paper is not adequate I believe, as the only statement I could find was "The package is written in pure Julia to take advantage of the language performance". Just using Julia is not enough to bring optimal performance versus e.g., Python. Did the authors take any steps/considerations when creating the ODE problem to be passed to DifferentialEquations.jl to maximize performance? Did they utilize caches, did they parallelize anything via threads or other means, etc.
- I did not find community guidelines in the docs.
Hi @Datseris
We agree that testing of physically correct results are needed. We now added such testing. Given the stochastic nature of the problem solved, we approached the problem from two different perspectives. First, we added a test at zero temperature of the solution of the set of differential equations. This is done to remove the stochastic component from the system and it is checked against the analytical solution that we derive in this regime. Second, we added a fixed-seed test. Such test is added to verify consistency of the solution when the stochastic component is present. We check the result against stochastic results found in the literature.
We understand the concerns of the reviewer about the name of the package. We note that such naming is derived a few years ago by the initial mission of the package. The mission was indeed to improve the classical simulation in the atomistic "Spin Dynamics" (ASD) community (see for example https://doi.org/10.1063/1.4930971). Hence, the name SpiDy. In time, we improved and updated the package to make it deal with different spectral densities, different noise, multiple spins, and even added a variant to solve systems of coupled harmonic oscillators. But, while the range of possible applications has expanded in time, the main applicative use of SpiDy.jl remains the atomistic spin dynamics community. For this reason, we believe the the name is still relevant and we would like to keep it as is.
We agree that the sentence of performance indicated by the reviewer is not backed up by any numerical comparison or asymptotics. What we wanted to express was intrinsically related to the use of the DifferentialEquations.jl package in Julia. Such package is notoriously an efficient package for solving sets of differential equations, allowing for vast method customizations at the same time. We followed the package performance guidelines during our implementation reducing evaluation redundancy, using callbacks, and preallocation (get_tmp to make it type-stable and non-allocating). To make the text more precise and consistent we changed the sentence to "The package is written in pure Julia and we take advantage of the efficient DifferentialEquations.jl package by reducing evaluation redundancy, using callbacks, and pre-allocations".
Thank you for reminding us. We completed and added all the documents needed to meet the community guidelines.
Hi @matthewfeickert, we have addressed all the issues and comments of the reviewers and answered accordingly
The authors have addressed all my concerns so I'm happy to recommend publication!
That authors have fully addressed my concerns. I am happy to recommend publication!
@Datseris, @mdavezac Thank you very much for your reviews!
@dawbarton Please provide us with an update on the status of your review. Are you near completion of it? If so then we will wait for it, but if you will not have your review finished by 2024-05-04 then we will drop your review so that we do not delay the authors any more.
@mekise After we have all the reviews in I will start the final editorial review process and we can move quickly towards final revisions and publication.
@matthewfeickert please move on without my review - it looks like you’ve already got enough good reviews.
Sounds good, @dawbarton.
@editorialbot remove @dawbarton from reviewers
@dawbarton removed from the reviewers list!
Hi @matthewfeickert . Please let me know if I need to do anything else. Thank you for your editing!
Hi @matthewfeickert ! Do we have any update on the final editorial review? Thank you!
@mekise Thank you all for your patience with my lack of responsiveness (I have been traveling for work internationally for the last weeks and my email is unfortunately quite a mess at the moment). I have returned as of today, so I will start the sequence now with @editorialbot and then post any editorial Issues by the end of the day tomorrow.
@editorialbot set <DOI here> as archive
@editorialbot set <version here> as version
@editorialbot generate pdf
@editorialbot check references
and ask author(s) to update as needed@editorialbot recommend-accept
@editorialbot generate pdf
Submitting author: !--author-handle-->@mekise<!--end-author-handle-- (Stefano Scali) Repository: https://github.com/quantum-exeter/SpiDy.jl Branch with paper.md (empty if default branch): main Version: v1.0.0 Editor: !--editor-->@matthewfeickert<!--end-editor-- Reviewers: @Datseris, @mdavezac Archive: 10.5281/zenodo.11283978
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
@Datseris & @mdavezac & @dawbarton, 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 @matthewfeickert 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 @Datseris
📝 Checklist for @dawbarton
📝 Checklist for @mdavezac