Closed editorialbot closed 1 year ago
editorialbot
commented
2 years 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 commandsFor example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdfSoftware report:
github.com/AlDanial/cloc v 1.88 T=0.02 s (772.8 files/s, 102570.2 lines/s)
---------------------------------------------------------------------------------------
Language files blank comment code
---------------------------------------------------------------------------------------
Rmd 6 292 401 947
R 1 5 2 84
TeX 1 0 0 81
Markdown 2 12 0 47
Bourne Shell 2 11 2 33
Windows Module Definition 1 2 0 29
YAML 1 1 4 18
Bourne Again Shell 1 5 3 12
---------------------------------------------------------------------------------------
SUM: 15 328 412 1251
---------------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repositoryReference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1093/molbev/msu300 is OK
- 10.1093/bioinformatics/btu153 is OK
- 10.1186/s13059-020-02090-4 is OK
- 10.1371/journal.pone.0163962 is OK
- 10.1099/mgen.0.000056 is OK
- 10.1038/nbt.3820 is OK
- 10.1186/s13059-014-0524-x is OK
- 10.1093/nar/gku1196 is OK
MISSING DOIs
- None
INVALID DOIs
- NoneWordcount for paper.md is 971
csoneson
commented
2 years ago 👋🏼 @hkaspersen, @mberacochea, @hseabolt, @rcannood - this is the review thread for the submission. All of our communications will happen here from now on.
Please check the post at the top of the issue for instructions on how to generate your own review checklist. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.
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 directly in the software repository. If you do so, please mention this thread so that a link is created (and I can keep an eye on what is happening). Please also feel free to comment and ask questions in this thread. It is often easier to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.
Please feel free to ping me (@csoneson) if you have any questions or concerns. Thanks!
editorialbot
commented
2 years ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
hseabolt
commented
2 years ago 
rcannood
commented
2 years ago 
mberacochea
commented
2 years ago hseabolt
commented
2 years ago @csoneson I have completed my initial review and have updated the review checklist in this thread as much as I am able to verify. I do have additional specific comments for the authors -- should those comments and my recommendation be posted here directly or should those be sent to you to collect and share with the authors?
Thanks!
csoneson
commented
2 years ago @hseabolt Thank you! Please post the comments in this thread, or feel free to open issues directly in the software repository if that is easier (just mention this thread there so that we can keep an eye on what's going on).
csoneson
commented
2 years ago @mberacochea, @rcannood - could you update us on the state of your reviews? Thanks!
hseabolt
commented
2 years ago @csoneson Thank you! My review is below.
The Nextflow code looks overall well designed and implemented -- why did the authors choose to include Shell wrapper scripts around Nextflow rather than just calling Nextflow and specifying the CLI arguments directly? This Shell wrapper layer is not typical of Nextflow pipelines and in my view, since these do not contain clear GetOpts or help messages for the user to understand them, they reduce the usability of this pipeline.
This pipeline shares alot of overlap in functionality with the nf-core Bactmap pipeline (https://nf-co.re/bactmap), and it is not clear in the manuscript text how this pipeline's feature set or functionality compares to Bactmap or other similar pipelines. It should be included in the text which new or different features set this pipeline apart from other ones currently available.
Unfortunately, while I am able to successfully install the pipeline and obtain the example test data, I cannot verify successfully running the pipeline with any of the three tracks using either test.sh or alppaca.sh (additional details below).
Specific comments I have are detailed below.
The installation and how-to-run documentation for this pipeline are not mentioned either on its main Github page nor in the manuscript. I did find the full documentation on Github as a Git Wiki, but please consider adding clear references to where this documentation can be found on the main Github page and/or in the manuscript text.
Please include some additional detail on what solution(s) this pipeline is intended to offer in README on the main Github repo. This could easily be adapted from the Statement of Need in the manuscript text, which is nicely written.
Please consider creating a conf folder in the main repo to hold additional custom configs users may wish to create (e.g. for other HPC schedulers, test profiles, etc). Please additionally consider reorganizing the files in the main directory to more appropriate locations (e.g. .sh files should relocate to bin, .config files should be placed in conf with the exception of nextflow.config.
Excellent examples of this style of repo organization can be found on any of the nf-core pipelines, e.g. viralrecon: https://github.com/nf-core/viralrecon
I am not a fan of the user having to manually download container images and manually update the Nextflow config files, as I feel this defeats one of the key shareability aspects of using Nextflow. DSL2 functionality enables container handling within modules, which would also have a secondary benefit of not needing to edit nextflow.config or either of the config files for the Shell wrappers.
This pipeline is configured to only work with Singularity and Slurm, however Nextflow has native support for a number of container engines and compute environments. While technically acceptable, I do feel this limitation detracts from the overall value of this work. I am not able to test the cluster profile since my HPC runs on Grid Engine.
Please consider including a Conda env.yml in the repo that contains the Nextflow and Java versions required to successfully run this pipeline. This will assist users who wish to run on HPC cluster environments but who may not have proper permissions or experience with installing software like Java in shared compute environments.
The example data was able to be downloaded successfully.
The test.sh script does not work 'out of the box' due to the Nextflow binary being called nextflow_21.10.6 on Line 24 of that script. This should be updated to just nextflow since most users will not have their executables named consistently with the authors' system.
Even with this minor change, the test.sh program does not execute successfully since the test.config file contains hard-coded filepaths on the authors' system. The user is not instructed to update these in the relevant documentation sections.
I tested all three tracks in the same manner following instructions given here: https://github.com/NorwegianVeterinaryInstitute/ALPPACA/wiki/1.-Installation.
The same execution issues arise when trying to run the alppaca.sh script, since again there are hard-coded paths in the main.config file. I have not made substantial effort to update these filepaths since there are several of them in each config file and this is a key barrier to the usability of ALPPACA -- in my judgement, most users will not go to the effort of manually updating these configs for every run and will simply look for a different solution.
For Shell scripts in the main directory and in bin, please update the shebang lines to #!/usr/bin/env bash, which is more flexible for different environments your users may have.
My recommendation is for the authors to make minor to moderate revisions (mainly to the codebase) and resubmit this manuscript and code repository.
mberacochea
commented
2 years ago @mberacochea, @rcannood - could you update us on the state of your reviews? Thanks!
Busy last couple of weeks. I'm aiming to get the review done this week.
mberacochea
commented
2 years ago ALPPACA is a well written Nextflow pipeline, the code is clean and easy to follow. The paper is very well written and clearly states the problem the authors are trying to solve.
The documentation of the software is stored in the GitHub Wiki section, but there are no links to it on either the manuscript or the repo README. Less experienced users will struggle to find it, please add a link to the README.
It would also be beneficial to use Read The Docs, Github Pages or a similar service to maintain the documentation and the output examples (which are a great resource).
Overall, the documentation is well written and covers the installation, functionality, and execution instructions of the software.
As previously stated, the Nextflow code is well-designed and implemented. But, there are some hardcoded bits which require of some manual tweaking before running the pipeline.
Some of the hardcoded values:
Please consider removing the paths and other hardcoded values.
Bash wrappers are used to run the pipeline (alppaca.sh and test.sh), this is an unusual pattern on Nextflow pipelines. In my experience such aproach is detrimental to the user experience. Also, the wrappers don't handle the arguments (e.g. GetOps) or have a help message.
The documentation makes emphasis on Slurm as the scheduler, but Nextflow supports a wide range of Executors (https://www.nextflow.io/docs/latest/executor.html#executors). As it stands the only requirements are Nextflow and Singularity. I think it would be beneficial to follow nf-core configuration practices (https://nf-co.re/usage/configuration#basic-configuration-profiles). Such approach allows users to customize the execution of the pipeline easily and following a well establish practice.
I also consider that following an improved configuration scheme will help manage the containers, at the moment users need to download them manually which is not very user-friendly.
Following the instructions on the documentation (after fixing the nextflow hardcoded binary in test.sh) it was possible to run the pipeline on my laptop. I do consider that supporting Docker or Podman will make it the pipeline more portable, albeit Singularity is a popular containers tech on HPC clusters.
Please, include the Nextflow, Singularity and Java versions required to run the pipeline.
My recommendation is for the authors to make minor revisions and resubmit the application.
rcannood
commented
2 years ago @mberacochea, @rcannood - could you update us on the state of your reviews? Thanks!
I created an issue in the ALPPACA repository. @csoneson Feel free to chime in on whether some of these comments are not required for the manuscript's acceptance at JOSS.
csoneson
commented
2 years ago @mberacochea, @hseabolt, @rcannood - thanks a lot for your thorough reviews! @hkaspersen - please have a look at the reviewers' comments, and let us know if you have questions or when you are ready for them to take a second look. Thanks!
hkaspersen
commented
2 years ago Dear @mberacochea, @hseabolt, @rcannood, and @csoneson. Thank you so much for taking the time for this review, your feedback is invaluable! I am relatively new to Nextflow, so this will be a learning process for me. The feedback regarding the Nextflow standards is especially useful - I will try to make the pipeline more in line with these standards.
If you have any useful resources to share regarding Nextflow standards, please do not hesitate to share! Thanks in advance!
@csoneson is there a time limit for addressing these changes?
hkaspersen
commented
2 years ago Also, should the changes made to the pipeline based on the comments above be registered as issues on the ALPPACA github page?
csoneson
commented
2 years ago @csoneson is there a time limit for addressing these changes?
No direct time limit - in order to respect the reviewers' time I'd just like to make sure that the submission is indeed "actively worked on" (i.e., not set aside and stalled for months without updates). It would be great to have a few updates along the way and perhaps an estimate of when you think there may be a new version for the reviewers to look at, but no stress - the main goal is to have a piece of software that you're happy with 🙂
Also, should the changes made to the pipeline based on the comments above be registered as issues on the ALPPACA github page?
That's one option, that makes it easy to track - you can also summarize the changes made here afterwards (and maybe point to specific commits addressing them).
Feel free to ping me at any time if you have questions!
hkaspersen
commented
2 years ago @csoneson can I update the version of the pipeline based on the changes I have made here without it affecting the review?
hkaspersen
commented
2 years ago Dear @mberacochea, @hseabolt, and @rcannood.
I am currently working on implementing some sort of test for the pipeline, but I am having a difficult time in figuring out how to do it the best way. If you have any suggestions or examples, please let me know! I see that nf-core provide tests that run on github actions, but I am not sure if I will manage to implement this here, as I am not familiar with this functionality.
csoneson
commented
2 years ago @csoneson can I update the version of the pipeline based on the changes I have made here without it affecting the review?
Yes - the version in the first post above is the version at the time of submission. You can update during the review process (but it won't be updated in this thread), and at the time of acceptance we sync the two again.
hseabolt
commented
2 years ago @hkaspersen you can include some small test data files in the assets dir in your repo, and create some test.config files in conf dir. Check out the example I have here: https://github.com/CDCgov/mycosnp-nf to see the file structure and examples of the test.config file. You will need to import this config into your nextflow.config in the base dir, examples of this can be seen in the MycoSNP nextflow.config, Line 95.
If you set this up correctly, you can activate the tests at runtime with nextflow run NorwegianVeterinaryInstitute/ALPPACA -profile test [other args] and it shouldn't require any Github actions. Note that you can give a multiple profiles to profile as a csv, e.g. -profile docker,test.
hkaspersen
commented
2 years ago @hseabolt thank you for your swift reply! I have looked into the code, but I cannot seem to understand how nextflow download the data from github and puts it into a channel - I see you have a samplesheet as input, but how is this file actually handled by nextflow? Thanks in advance!
hkaspersen
commented
2 years ago @csoneson how do I format bacterial names correctly in the .bib file, they are not in italics in the preview pdf. Can I use \textit bibtex formatting?
csoneson
commented
2 years ago Hi @hkaspersen - yes, I believe you should be able to use \textit in the titles in your .bib file.
hkaspersen
commented
2 years ago Dear @csoneson @rcannood @mberacochea @hseabolt.
Thank you so much for your valuable feedback on ALPPACA!
I have now worked on the changes mentioned by you all, and created a new release of ALPPACA (v.2.0.0). The changes are reflected in the issues mentioned above, and in the release description. @rcannood I will keep your issue open until the review is completed.
The main changes include the following:
nextflow run main.nf or directly from github using nextflow run NorwegianVeterinaryInstitute/ALPPACAcsv file with sample ids and full paths to all files used. This file can be created with the script supplied at bin/generate_input.R-profile)-profile testnextflow.configyml files added which users can use to generate a nextflow conda environment for running the pipelineThe paper has also been updated to reflect your suggestions above.
@csoneson let me know if I have to supply more information here! Looking forward to hear from you again!
csoneson
commented
2 years ago Thank you @hkaspersen!
@mberacochea, @hseabolt, @rcannood - could you have a look at the revised submission when you have a chance, and let us know what you think - please update your checklists accordingly and provide any additional comments as before.
Thanks everyone!
csoneson
commented
2 years ago @editorialbot generate pdf
editorialbot
commented
2 years ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
hkaspersen
commented
2 years ago @csoneson how strict is JOSS on the structure of the paper? I'm having some issues with the sections and the max word limit - and also trying to avoid repeating myself too much. As far as I have understood, the Summary section is not equivalent to an Abstract. In my manuscript though it sort of is intertwined with the statement of need. Is it possible to merge these two into one summary? Or is it mandatory to include both headers?
hkaspersen
commented
2 years ago Just to update the other reviewers: I have changed the structure of the paper to reflect the suggestions from @rcannood, and also made the pipeline test run automatically on push to the dev branch.
rcannood
commented
2 years ago @hkaspersen Are you able to generate the pdf or is this something only @csoneson can do?
hkaspersen
commented
2 years ago @editorialbot generate pdf
editorialbot
commented
2 years ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
csoneson
commented
2 years ago @csoneson how strict is JOSS on the structure of the paper? I'm having some issues with the sections and the max word limit - and also trying to avoid repeating myself too much. As far as I have understood, the Summary section is not equivalent to an Abstract. In my manuscript though it sort of is intertwined with the statement of need. Is it possible to merge these two into one summary? Or is it mandatory to include both headers?
@hkaspersen - I'd say that the most important is that both of these sections, with different purposes, are covered in the paper (from the docs):
Often it may be easier to provide them as separate sections. However, if you think that it can be well structured under a joint header ('Summary and Statement of Need'), that would cover both these aspects, that is also acceptable.
rcannood
commented
2 years ago @csoneson I just checked the last checkbox in my review issue and would like to recommend this paper for publication.
The speed and yet also rigorousness with which @hkaspersen managed to revise the manuscript and source code after receiving an extensive list of comments and suggestions from myself and the other reviews is quite exemplary. In particular, I really liked the way that he split up my comments in different issues which made it really easy for me to follow what changes were made to address the various comments.
csoneson
commented
2 years ago Thank you @rcannood!
hkaspersen
commented
2 years ago @rcannood thank you so much for your kind words! Your comments were indeed helpful, and I learned a lot!
Just to inform: I bumped the version of the pipeline to 2.0.1. This was due to the addition of the core gene and mapping track tests. Now all three tracks are run when i push to dev
hseabolt
commented
2 years ago I will plan to complete this review over the weekend
hseabolt
commented
1 year ago @csoneson I have completed my review and updated the checklist which now covers all review items. I would like to recommend this manuscript for publication and would additionally like to echo the excellent comments by @rcannood regarding the quality of the updated work. Kudos to @hkaspersen -- well done!
csoneson
commented
1 year ago Thank you @hseabolt!
hkaspersen
commented
1 year ago @hseabolt Thank you for your kind words!
mberacochea
commented
1 year ago @csoneson I have completed my review. I agree with the other reviewers, and I recommend this manuscript for publication. Excellent work from the authors, it was easy to follow the changes in the context of the comments from the initial review. Kudos to @hkaspersen, brilliantly done.
csoneson
commented
1 year ago Thank you @mberacochea!
@hkaspersen - I will also take a quick look through the submission, and I'll get back to you shortly with the next steps.
csoneson
commented
1 year ago @editorialbot generate pdf
csoneson
commented
1 year ago @editorialbot check references
editorialbot
commented
1 year ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1093/molbev/msu300 is OK
- 10.1093/bioinformatics/btu153 is OK
- 10.1186/s13059-020-02090-4 is OK
- 10.1371/journal.pone.0163962 is OK
- 10.1099/mgen.0.000056 is OK
- 10.1038/nbt.3820 is OK
- 10.1186/s13059-014-0524-x is OK
- 10.1093/nar/gku1196 is OK
- 10.1128/AEM.02769-19 is OK
- 10.3389/fmicb.2021.725414 is OK
- 10.3168/jds.2021-21471 is OK
- 10.1016/j.vetmic.2022.109378 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:
Submitting author: !--author-handle-->@hkaspersen<!--end-author-handle-- (Håkon Kaspersen) Repository: https://github.com/NorwegianVeterinaryInstitute/ALPPACA Branch with paper.md (empty if default branch): joss_paper Version: v2.0.1 Editor: !--editor-->@csoneson<!--end-editor-- Reviewers: @mberacochea, @hseabolt, @rcannood Archive: 10.5281/zenodo.7331351
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
@mberacochea & @hseabolt & @rcannood, 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 @csoneson 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 @hseabolt
📝 Checklist for @rcannood
📝 Checklist for @mberacochea