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

Compute raw activity profile #162

Merged
merged 4 commits into from
Oct 1, 2021
Merged

Conversation

joergi-w
Copy link
Member

Resolves #152: Scans a sam file and creates an activity vector per reference genome.

Solves also part of #151, as it introduces a parameter for a genome filename (the file is not yet read). If a genome file name is given, iGenVar switches to the SNP/Indel calling mode.

There are no tests yet for this functionality.

@codecov
Copy link

codecov bot commented Sep 27, 2021

Codecov Report

Merging #162 (b2587a5) into master (a359706) will increase coverage by 0.09%.
The diff coverage is 98.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #162      +/-   ##
==========================================
+ Coverage   96.39%   96.49%   +0.09%     
==========================================
  Files          18       19       +1     
  Lines         749      798      +49     
==========================================
+ Hits          722      770      +48     
- Misses         27       28       +1     
Impacted Files Coverage Δ
src/variant_detection/variant_detection.cpp 93.97% <ø> (-0.15%) ⬇️
src/variant_detection/snp_indel_detection.cpp 97.56% <97.56%> (ø)
src/iGenVar.cpp 98.34% <100.00%> (+0.14%) ⬆️

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

@Irallia
Copy link
Collaborator

Irallia commented Sep 27, 2021

This looks like you need a test for the new error message. I just reordered all the CLI tests: #164 maybe wait with adding the test for this PR?
But you can already ask for the first review I think :)

@joergi-w
Copy link
Member Author

Thanks! I'll wait for the pending PR and then update this one on Wednesday.

@joergi-w joergi-w requested review from a team and SGSSGene and removed request for a team September 29, 2021 09:33
Copy link

@SGSSGene SGSSGene left a comment

Choose a reason for hiding this comment

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

Could add a few more file extensions, but looks good to go.

src/iGenVar.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@Irallia Irallia left a comment

Choose a reason for hiding this comment

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

Many, many thanks for this PR!
Do not be put off by the many comments. They are mainly small things and questions.

Comment on lines 6 to 7
* \brief Detect Single Nucleotide Polymorphisms (SNPs) and short deletions and insertions.
* \param reads_filename The file path where to find the sequenced reads.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* \brief Detect Single Nucleotide Polymorphisms (SNPs) and short deletions and insertions.
* \param reads_filename The file path where to find the sequenced reads.
* \brief Detect Single Nucleotide Polymorphisms (SNPs) and short deletions and insertions (Indels).
* \param[in] reads_filename - The file path where to find the sequenced reads.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we put this hyphen? I don't find the meaning in the Doxygen manual for \param. I see it just ends up as the beginning of the description string.

Copy link
Member Author

Choose a reason for hiding this comment

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

html_doc

pdf_doc

Copy link
Collaborator

Choose a reason for hiding this comment

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

We once decided on this style, either we decide to change in general, or stay consistent. -> https://github.com/seqan/iGenVar/wiki/Coding-Guide
I find it clearer in the first image, but yes it looks a bit unnecessary in the second image. So if you think the change is important, then we should discuss it again and change it in a separate PR.

src/iGenVar.cpp Outdated Show resolved Hide resolved
src/iGenVar.cpp Outdated Show resolved Hide resolved
src/variant_detection/snp_indel_detection.cpp Show resolved Hide resolved
src/variant_detection/snp_indel_detection.cpp Outdated Show resolved Hide resolved
src/variant_detection/snp_indel_detection.cpp Outdated Show resolved Hide resolved
test/cli/CMakeLists.txt Show resolved Hide resolved
Comment on lines +317 to +319
"Start clustering...\n"
"Done with clustering. Found 0 junction clusters.\n"
"No refinement was selected.\n";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question for the next meeting: Do we have our own clustering method here or do we use the existing one? Do we need the refinement for SNPs at all?

Copy link
Collaborator

@Irallia Irallia left a comment

Choose a reason for hiding this comment

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

Thanks! ❤️

@Irallia Irallia merged commit c8d47ab into seqan:master Oct 1, 2021
@joergi-w joergi-w deleted the snp_indel_call branch October 7, 2021 07:45
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.

Calculate a raw activity profile
3 participants