[JOSS] @mberacochea's feedback for the JOSS submission

[mberacochea]

Hello,

This ticket is to gather my review comments for the JOSS submission (openjournals/joss-reviews#5397).

Functionality

Installation

• Ideally the only required software to run the pipeline should be Nextflow and Docker/Singularity. I would suggest review the installation instructions, requiring a conda env. shouldn't be required. #36
  • Added a comment -> Ideally the only required software to run the pipeline should be Nextflow and Docker/Singularity. I would suggest review the installation instructions, requiring a conda env. shouldn't be required. #36
• A nice UX pattern is to have the download_dbs.py to be part of the pipeline itself. It's possible to have a module to download the dbs, if they are missing. This lowers the entry barrier in my experience. #41

Functionality

TODO: I will run the pipeline once the installation instructions have been amended.

Documentation

A statement of need

• It would be useful to have a bit of more information in the README. #27
  • README greatly improved -> Clearer parameters documentation (fix #19 #21 #27 #34 #35 #36 #37) #22

Installation instructions

• This is linked to my previuos comment in the "Functionality" entry. Nextflow + Singularity/Docker should be the only requirements to run the pipeline #37
  • README greatly improved, with better installation instructions -> Clearer parameters documentation (fix #19 #21 #27 #34 #35 #36 #37) #22

Automated tests

• There are no automated tests. My recommendation is to use like nf-test or pytest-workflow. By doing this, the functionality of the pipeline can be easily tested. This introduces some extra work, so I would recommend adding at least one nf-test or pytest-worklow test for the whole pipeline. If this is not feasible, please provide some documentation around the test data set included in the repo, specially around the output files. Which output files are the key ones, and how should they look like. #48

Community guidelines

• There are no community guidelines. #28
  • Added -> https://github.com/jhayer/baargin/pull/25/files

Software paper

State of field

• This is missing from the software paper #30
  • Added -> dd2e39f

Quality of writing

• Line 61: instead of " The contigs are taxonomically assigned using Kraken2.." -> "Taxonomic assignment of the contigs is performed using Kraken2..."
• Line 67: instead of "A quality check of the assembly is achieved by Quast..." -> "A quality check of the assembly is conducted using Quast...".
• Line 73: "The contigs are then submitted to a plasmid identification with PlasmidFinder" -> "The contigs are subsequently submitted to plasmid identification using PlasmidFinder."
• Line 98: " that belong the expected taxon" -> "that belong to the expected taxon"
• Nextflow is written as "NextFlow" and "Nextflow" throughout the text.

Other comments

Overall the nextflow code is correct and easy to follow. I would recommend to split the pipeline into several sub-workflows, that helps with the readability.

• There are some nextflow modules with strange identation, such as https://github.com/jhayer/baargin/blob/main/modules/agat.nf #31
  • Fixed -> remove un-needed modules (fix #31 and #32) #33
• The params are available in the modules, there is no need to provide that on import (such as: include {quast} from './modules/quast.nf' params(output: params.output) #38
  • Fixed remove un-needed params on import (fix #38) #39
• I recommend to remove unused-commented code and unused modules (such as https://github.com/jhayer/baargin/blob/main/modules/emblmygff3.nf) #32
  • Fixed -> remove un-needed modules (fix #31 and #32) #33
• The containers.config file should not have the cpus, memory and time limits for the modules. Placing those in a base.config or similar makes it easier to find the module's resource requirements. #42
  • Fixed -> create base config for time and cpus (fix #42) #43
• Python: for tabular results it's recommended to use the stdlib csv module. This makes the code easier to follow as the csv module is responsable for the formatting. Example -> https://github.com/jhayer/baargin/blob/main/bin/amrfinder_compile_heatmap.py#L79 #45
  • Fixed -> https://github.com/jhayer/baargin/pull/46]

[mberacochea]

OK, my first review is completed now. @jhayer please, if you disagree with any of my comments please let me know :).

[jhayer]

Thank you for your review @mberacochea. We will go through all the comments and fix everything by the end of next week.

[jhayer]

Hi @mberacochea,
We now finally went through all your comments and fixed everything. Thank you for your valuable input.

[mberacochea]

Hi @jhayer ... apologies it took me so long to reply.
I'm running the test dataset now, I'll post an update as soon as I have the results of that.

[mberacochea]

Well.. I was able to run the test data set. I think this PR can be closed :)