Skip to content

Added an abyss component to flowcraft, and added a new disableRR parameter to spades#146

Merged
ODiogoSilva merged 21 commits intoassemblerflow:devfrom
emreerhan:abyss
Oct 13, 2018
Merged

Added an abyss component to flowcraft, and added a new disableRR parameter to spades#146
ODiogoSilva merged 21 commits intoassemblerflow:devfrom
emreerhan:abyss

Conversation

@emreerhan
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Oct 13, 2018

Codecov Report

Merging #146 into dev will increase coverage by 0.04%.
The diff coverage is 58.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #146      +/-   ##
==========================================
+ Coverage   38.92%   38.97%   +0.04%     
==========================================
  Files          59       59              
  Lines        5636     5647      +11     
==========================================
+ Hits         2194     2201       +7     
- Misses       3442     3446       +4
Impacted Files Coverage Δ
flowcraft/generator/engine.py 91.33% <ø> (ø) ⬆️
flowcraft/templates/spades.py 0% <0%> (ø) ⬆️
flowcraft/generator/components/assembly.py 100% <100%> (ø) ⬆️

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 6bf0971...1972917. Read the comment docs.

Copy link
Collaborator

@ODiogoSilva ODiogoSilva left a comment

Choose a reason for hiding this comment

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

Great contribution @emreerhan! Very thorough with the addition of logging and documentation for new parameters 👍 I've only added one simple comment. Will also change the PR to dev instead of master as this is common practice in flowcraft. We only merge to master for new releases.

self.input_type = "fastq"
self.output_type = "fasta"

# self.dependencies = ["integrity_coverage"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's OK to remove this line altogether

@ODiogoSilva ODiogoSilva changed the base branch from master to dev October 13, 2018 21:34
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.

Nice work, just had some minor comments. Maybe @ODiogoSilva has something to say about the spades modifications but they looked fine to me.

self.input_type = "fastq"
self.output_type = "fasta"

# self.dependencies = ["integrity_coverage"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need this line? If you don't need it, remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove it!

"true_coverage": readsqc.TrueCoverage,
"viral_assembly": assembly.ViralAssembly
"viral_assembly": assembly.ViralAssembly,
"abyss": assembly.Abyss
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although this will probably be refactored according to #145 , can you keep the alphabetical order for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@emreerhan
Copy link
Contributor Author

I accidentally pushed my changes to spades to this fork. I'll edit the title of the PR to reflect this. Is there anything else I should do too?

@emreerhan emreerhan changed the title Added an abyss component to flowcraft Added an abyss component to flowcraft, and added a new disableRR parameter to spades Oct 13, 2018
@ODiogoSilva
Copy link
Collaborator

Yes, there is one more thing. Could add these changes to the changelog.md? The new components should be under a header of ### New components and the changes to existing components with ### Components changes

@ODiogoSilva
Copy link
Collaborator

Sooooorry @emreerhan 😅 Could you put the change log entries in the Changes in upcoming release (dev branch)? They are currently under the 1.3.1 release, which already occurred. You'll need to add the headers as well.

@emreerhan
Copy link
Contributor Author

Oops!

@ODiogoSilva
Copy link
Collaborator

Welcome to the team @emreerhan 💪 The docker image for abyss will also be up in dockerhub shortly, after the PR is merged.

@ODiogoSilva ODiogoSilva merged commit 6034a2e into assemblerflow:dev Oct 13, 2018
@tiagofilipe12
Copy link
Collaborator

Good job and welcome to the team @emreerhan . Very nice PR. Hope you have fun with FlowCraft!

@sjackman sjackman mentioned this pull request Oct 14, 2018
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants