joss-reviews
joss-reviews copied to clipboard
openjournals
[REVIEW]: TimeseriesSurrogates.jl: a Julia package for generating surrogate data
Submitting author: @kahaaga (Kristian Agasøster Haaga) Repository: https://github.com/JuliaDynamics/TimeseriesSurrogates.jl Branch with paper.md (empty if default branch): main Version: v2.0.0 Editor: @jarvist Reviewers: @lucaferranti, @dawbarton Archive: Pending
Status
Status badge code:
HTML: <a href="https://joss.theoj.org/papers/8145914a409945b642e6fd6bd72c8d6a"><img src="https://joss.theoj.org/papers/8145914a409945b642e6fd6bd72c8d6a/status.svg"></a>
Markdown: [](https://joss.theoj.org/papers/8145914a409945b642e6fd6bd72c8d6a)
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
@lucaferranti & @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:
@editorialbot generate my checklist
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @jarvist 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
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 (1331.7 files/s, 114874.4 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Julia 27 552 221 2124
Markdown 18 204 0 684
MATLAB 3 128 200 480
YAML 6 8 11 177
TeX 1 11 0 144
TOML 3 5 0 54
-------------------------------------------------------------------------------
SUM: 58 908 432 3663
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repository
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1016/j.physrep.2018.06.001 is OK
- 10.1137/141000671 is OK
- 10.1016/0167-2789(92)90102-s is OK
- 10.1016/0375-9601(94)91096-0 is OK
- 10.1103/physrevlett.77.635 is OK
- 10.1121/1.4929694 is OK
- 10.1103/physreve.74.026205 is OK
- 10.1103/physreve.73.036707 is OK
- 10.1103/physrevlett.87.188101 is OK
- 10.1103/physreve.59.4044 is OK
- 10.1103/physrevlett.101.134101 is OK
- 10.1103/physrevlett.81.4345 is OK
- 10.1103/physreve.85.056202 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:
@dawbarton @lucaferranti - just confirming that you both got invited to this issue for the review!
Review checklist for @lucaferranti
Conflict of interest
- [x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.
Code of Conduct
- [x] I confirm that I read and will adhere to the JOSS code of conduct.
General checks
- [x] Repository: Is the source code for this software available at the https://github.com/JuliaDynamics/TimeseriesSurrogates.jl?
- [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
- [x] Contribution and authorship: Has the submitting author (@kahaaga) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
- [x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
Functionality
- [x] Installation: Does installation proceed as outlined in the documentation?
- [x] Functionality: Have the functional claims of the software been confirmed?
- [x] 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
- [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
- [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
- [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
- [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
- [x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
- [x] 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
- [x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
- [x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
- [x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
- [x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
- [x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?
@jarvist I didn't know reviewers have to generate their checklist, is it new? Anyway I generated my checklist and looks like I can edit, so I guess everything is setup? :)
Hi @kahaaga (cc @jarvist ) overall a good submission! Here is the first batch of comments related to the code
Note: This PR is based on the code and docs in the main branch. I noticed you have a PR "JOSS paper" that with a quick look seems to address some of the comments I have below, could you clarify the status of that PR? Maybe it can be merged if it's ready?
Functionality
- [ ] At the moment installation instructions are missing, it would be good to have those both in the README and in the docs homepage (see below)
- [ ] The paper has some benchmarks, it would be good to include the code to reproduce those benchmarks. I guess there are several way to approach this, one idea could be to have a benchmarks article also in the documentation, where code snippets and results are included and explained and/or having a benchmark folder with e.g. 1) a julia file for the julia experiments 2) a matlab file for the matlab experiments 3) a postprocessing file for generating the results and graphs (this could be together with 1).
Documentation
- [ ] Several requirements are currently, missing e.g. installation instructions, contributing guidelines etc., please see the checkpoints in the list above.
- [ ] Regarding the structure, currently the homepage goes straight into the API docs, this could be a little overwhelming for a newcomer, I would suggest the homepage of the docs to include the followings (but feel free to add remove items if you prefer), it would also be good to have a more or less similar structure in the readme too.
- A general overview of the package
- installation instructions
- A small quickstart example, something small and easy to get the taste of the package
- [ ] Currently the readme has only the link to the docs of the dev version but not to the latest stable release.
Other
a few minor comments
- [ ] the CI badge in the readme points to travis, but looking at the PRs you seem to use github actions (indeed the latest action on travis seems to be one year old). I guess the travis badge is a leftover? In that case the
.travis.ymlcould be removed too probably? - [ ] (very minor, feel free to ignore if you like) I noticed you are already using codecov to compute the coverage. Since you have it already setup, have you considered adding a badge and link to the coverage report in the readme? The coverage is 88%, so it would also look good in the repo :)
This was a first batch of comments, I'll next read the paper more carefully and check I can reproduce the examples in the docs.
Hey @lucaferranti! Thanks for the initial comments.
This PR is based on the code and docs in the main branch. I noticed you have a PR "JOSS paper" that with a quick look seems to address some of the comments I have below, could you clarify the status of that PR? Maybe it can be merged if it's ready?
When submitting the paper, I had the option to select a branch to be considered for the publication. I chose the JOSSPaper branch, so that's the latest version of the paper/docs/etc. However, I see that it's more convenient if everything is on the master branch, so I merged it now. The documentation badge in the repo readme now points to the updated documentation.
I think that should address most of your initial comments. In particular:
The paper has some benchmarks, it would be good to include the code to reproduce those benchmarks. I guess there are several way to approach this, one idea could be to have a benchmarks article also in the documentation, where code snippets and results are included and explained and/or having a benchmark folder with e.g. 1) a julia file for the julia experiments 2) a matlab file for the matlab experiments 3) a postprocessing file for generating the results and graphs (this could be together with 1).
The benchmarks are now in the paper/timing/code folder of the repo.
Regarding the structure, currently the homepage goes straight into the API docs, this could be a little overwhelming for a newcomer, I would suggest the homepage of the docs to include the followings (but feel free to add remove items if you prefer), it would also be good to have a more or less similar structure in the readme too. 1) A general overview of the package 2) installation instructions 3) A small quickstart example, something small and easy to get the taste of the package
Please have a look at the updated documentation. It now includes installation instructions, a brief overview of the package (with a link to the dedicated "what is a surrogate" page).
As for the quickstart example, there are already plentyful of examples in the documentation - one for most of the methods. We'd prefer to keep the documentation as minimal as possible, without unnecessary repetition. Maybe it'd be enough to include a sentence in the opening paragraphs of the docs that examples are found under the individual method pages?
the CI badge in the readme points to travis
The CI badge now points to Github Actions, which are currently used.
(very minor, feel free to ignore if you like) I noticed you are already using codecov to compute the coverage. Since you have it already setup, have you considered adding a badge and link to the coverage report in the readme? The coverage is 88%, so it would also look good in the repo :)
A codecov badge is now included in the readme for the repo!
Dear @lucaferranti , thank you very much for your work so far!
@dawbarton , have you had a chance to look at this yet?
Apologies for the delay. I'll try to make sure that it gets done this week.
Review checklist for @dawbarton
Conflict of interest
- [x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.
Code of Conduct
- [x] I confirm that I read and will adhere to the JOSS code of conduct.
General checks
- [x] Repository: Is the source code for this software available at the https://github.com/JuliaDynamics/TimeseriesSurrogates.jl?
- [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
- [x] Contribution and authorship: Has the submitting author (@kahaaga) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
- [x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
Functionality
- [x] Installation: Does installation proceed as outlined in the documentation?
- [x] Functionality: Have the functional claims of the software been confirmed?
- [x] 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
- [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
- [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
- [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
- [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
- [x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
- [x] 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
- [x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
- [x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
- [x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
- [x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
- [x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?
I've now checked through the documentation and the paper. Overall, it looks good. The points previously raised by the other reviewer have been addressed and there isn't anything else substantive to address as far as I can see.
Two very minor things that I did pick up on:
- I tried the timing code - it was a bit annoying as it can't just be run without modification. Personally, I would separate out the data generation or the plotting code (or both) because at the moment you have to copy-and-paste the code in piecemeal because there is a circular dependency. (Matlab needs to use the same data that is generated in the Julia script but the Julia script also plots pre-saved Matlab results that can only be regenerated once you've got the data.) Also, the hardcoded paths are a bit annoying - you could use
@__FILE__or@__DIR__instead. - It might be worth putting a little more disambiguation of the term "surrogates" in the package readme / docs. It's a bit of an overloaded term with surrogate models being a significant area of interest at the moment (but completely different).
@jarvist Sorry for the delay!
We have now addressed @dawbarton's comments. The changes have been merged to the main branch.
I tried the timing code - it was a bit annoying as it can't just be run without modification. Personally, I would separate out the data generation or the plotting code (or both) because at the moment you have to copy-and-paste the code in piecemeal because there is a circular dependency. (Matlab needs to use the same data that is generated in the Julia script but the Julia script also plots pre-saved Matlab results that can only be regenerated once you've got the data.) Also, the hardcoded paths are a bit annoying - you could use @FILE or @DIR instead.
Good point. I now use @__DIR__ for file paths, and reorganised the timing scripts so that tasks are delegated a bit differently between MATLAB and Julia. Now, first run the MATLAB script (which will generate example data), then the Julia script (which will do the comparison and generate the plot for the paper).
I also increased the number of surrogates generated for the comparison for a bit more reliable timings (@jarvist note: the paper pdf must be re-generated to include the update timing plot).
It might be worth putting a little more disambiguation of the term "surrogates" in the package readme / docs. It's a bit of an overloaded term with surrogate models being a significant area of interest at the moment (but completely different).
The wording in the documentation was slightly adjusted to highlight that we are dealing with time series surrogates.
@kahaaga apologies for the delay for finishing my review, I will get it done today
Hello @lucaferranti, here are the things you can ask me to do:
# List all available commands
@editorialbot commands
# Get a list of all editors's GitHub handles
@editorialbot list editors
# Check the references of the paper for missing DOIs
@editorialbot check references
# Perform checks on the repository
@editorialbot check repository
# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist
# Set a value for branch
@editorialbot set joss-paper as branch
# Generates the pdf paper
@editorialbot generate pdf
# Get a link to the complete list of reviewers
@editorialbot list reviewers
@kahaaga @jarvist there is an error when generating the pdf, not sure if it's a bot issue or paper issue, can you have a look at it?
@lucaferranti I originally submitted the paper referring to the JOSSPaper branch, which no longer exists, because it was merged to main and deleted (see discussion above).
The error seems to be related to the fact that this branch no longer exists.
Maybe running @editorialbot set main as branch, then re-generating the pdf will work?
meanwhile @kahaaga, overview software and docs look good to me. As a small nitpick, currently the installation instructions in the docs homepage are last. This seems a little strange to me, I would suggest to move that section before the API section.
I will check the paper once I can generate the pdf.
Maybe running
@editorialbot set main as branch, then re-generating the pdf will work?
I don't think a reviewer should be allowed to change the branch where the paper is generated from, that sounds like something the author and/or editor should do, can you try it?