assemblerflow / flowcraft

FlowCraft: a component-based pipeline composer for omics analysis using Nextflow. :whale::package:
GNU General Public License v3.0
241 stars 44 forks source link

Add SeqSero2 and stx subtyping #190

Closed miguelpmachado closed 5 years ago

miguelpmachado commented 5 years ago

Add two SeqSero2 components for Salmonella serotyping (reads and assembly)

codecov-io commented 5 years ago

Codecov Report

Merging #190 into dev will increase coverage by 0.15%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #190      +/-   ##
==========================================
+ Coverage   43.96%   44.12%   +0.15%     
==========================================
  Files          67       67              
  Lines        6036     6058      +22     
==========================================
+ Hits         2654     2673      +19     
- Misses       3382     3385       +3
Impacted Files Coverage Δ
flowcraft/generator/recipe.py 85.71% <ø> (ø) :arrow_up:
flowcraft/generator/process.py 95.42% <ø> (ø) :arrow_up:
flowcraft/generator/components/typing.py 100% <100%> (ø) :arrow_up:
flowcraft/templates/trimmomatic.py 0% <0%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a55c431...d4f2a9f. Read the comment docs.

jacarrico commented 5 years ago

@tiagofilipe12 this is a similar case as Unicycler. Although it is not optimal it serves a purpose for testing and implementation , and it is needed for thr platform as is. Therefore accept this addition to FlowCraft since it is needed for the developed work.

tiagofilipe12 commented 5 years ago

@jacarrico I don't think it's the same. Unicyler is a pipeline that runs several programs indeed, but this is a pipeline that runs another pipeline that could be a flowcraft module. I don't agree with it in its current state, thus I will not accept it. Since I am not blocking it, you can accept it which should be enough to squash and merge to dev.

The code I think already has been improved by @miguelpmachado with the review, but I think that Rematch could be a separate module in flowcraft that could be used as dependency of this module, as we do for other modules, instead of being hidden within the docker image. I think it will be a win-win in the end, we win a new module and we also win transparency.

jacarrico commented 5 years ago

@tiagofilipe12 yes I see your point and I would love better modularity but in a perfect world I would also still have a team to sort this out in a timely fashion and I wouldn’t be leaving discussing semantics on a project that should have been done already in a Sunday early morning. As it this is not the case anymore, I need to work with what I have and as if FlowCraft is to be used in several scientific scenarios it will need modules that are pre built self contained pipelines that produce a single result as it is the case of SeqTyping. One doesn’t have the time on manpower to deconstruct all the software as it appears, specially as validating the same outputs as the original software is rather time consuming.

bfrgoncalves commented 5 years ago

This PR will be merged to a different branch (innuendo) #197