Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to Innuca's recipe modules #191

Open
wants to merge 20 commits into
base: dev
Choose a base branch
from
Open

Update to Innuca's recipe modules #191

wants to merge 20 commits into from

Conversation

cimendes
Copy link
Member

@cimendes cimendes commented Jan 22, 2019

This PR updates the modules in Innuca's recipe to incorporate the latest changes in the INNUca pipeline (https://github.com/B-UMMI/INNUca). Several components where updated and/or reworked including integrity_coverage, mlst, process_assembly and process_assembly_mapping. A new component, named "insert_size" was added. I apologize for the length of the PR. They made me do it! :P

Enjoy the code review.


This change is Reviewable

cimendes and others added 11 commits January 8, 2019 15:14
…s with the appropriate compressing software. Added new directive with container
…on of the expected species vs schema species

Co-authored-by: cimendes <[email protected]>
Co-authored-by: miguelpmachado <[email protected]>
- add sampel id to novel alleles file
If expected species parameter not provided, bypasses the species verification control.
@codecov-io
Copy link

codecov-io commented Jan 22, 2019

Codecov Report

Merging #191 into dev will decrease coverage by 2.37%.
The diff coverage is 3.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #191      +/-   ##
==========================================
- Coverage   43.96%   41.59%   -2.38%     
==========================================
  Files          67       69       +2     
  Lines        6036     6404     +368     
==========================================
+ Hits         2654     2664      +10     
- Misses       3382     3740     +358
Impacted Files Coverage Δ
flowcraft/generator/components/mlst.py 100% <ø> (ø) ⬆️
flowcraft/templates/spades.py 0% <ø> (ø) ⬆️
flowcraft/generator/recipes/innuca.py 100% <ø> (ø) ⬆️
flowcraft/templates/integrity_coverage.py 0% <0%> (ø) ⬆️
flowcraft/templates/process_assembly_mapping.py 0% <0%> (ø) ⬆️
flowcraft/templates/run_mlst.py 0% <0%> (ø)
flowcraft/templates/fastqc_report.py 0% <0%> (ø) ⬆️
flowcraft/templates/metaspades.py 0% <0%> (ø) ⬆️
flowcraft/templates/process_assembly.py 0% <0%> (ø) ⬆️
flowcraft/templates/insert_size.py 0% <0%> (ø)
... and 5 more

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...e62ee23. Read the comment docs.

set columBar to False in mlst .report.json
Copy link
Collaborator

@tiagofilipe12 tiagofilipe12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok don't get spoked by the number of comments or by the work to do.

In my opinion all nf / fc workings are fit to the purpose but the python templates are too sketchy and shady. There are a lot of coding patterns that are not scalable, easy to understand nor clear what it does. Improving documentation is a must, but I also recommend re-thinking the code regarding these string parsers with so many nested if statements. I have left you some suggestions but there is a lot going on that is simply impossible for me to understand without trial/error debugging the code.

Nice effort though when putting these scripts in script templates rather than running them directly from the shell. 💯

changelog.md Outdated Show resolved Hide resolved
changelog.md Outdated Show resolved Hide resolved
changelog.md Outdated Show resolved Hide resolved
flowcraft/generator/components/mapping.py Outdated Show resolved Hide resolved
flowcraft/generator/components/mapping.py Outdated Show resolved Hide resolved
pass_qc = True
logger.warning("Found a Yersinia scheme ({scheme_mlst}), but it is different from what it was"
" expected ({scheme})".format(scheme_mlst=scheme_mlst, scheme=scheme))
else:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if/elif/else is ok because you have to check for several different conditions

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, these conditions lack proper explanation of the rational.

flowcraft/templates/run_mlst.py Outdated Show resolved Hide resolved
flowcraft/templates/run_mlst.py Outdated Show resolved Hide resolved
flowcraft/templates/run_mlst.py Outdated Show resolved Hide resolved
flowcraft/templates/run_mlst.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants