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

Samtools: Stats: fix "Percent Mapped" plot when samtools was run with read filtering #1972

Merged
merged 5 commits into from
Aug 24, 2023

Conversation

lindenb
Copy link
Contributor

@lindenb lindenb commented Aug 9, 2023

This PR fixes a problem with samtools stats #1971

I'm not a python dev, so I hope it's ok !

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md has been updated

@lindenb lindenb changed the title Pl samtools stats fix bug when counting read in samtools stats (fix for https://github.com/ewels/MultiQC/issues/1971 ) Aug 9, 2023
forgot an underspace...
@ewels ewels changed the title fix bug when counting read in samtools stats (fix for https://github.com/ewels/MultiQC/issues/1971 ) fix bug when counting reads in samtools stats Aug 9, 2023
@ewels
Copy link
Member

ewels commented Aug 9, 2023

Thanks for this @lindenb!

It looks like this metric has been completely missed until now..

  • Is it always there? (It'll crash if not currently)
  • Are there any plots / tables that should include this as a category as well?

Cheers,

Phil

@lindenb
Copy link
Contributor Author

lindenb commented Aug 10, 2023

Is it always there? (It'll crash if not currently)

no, as far as I understand , the error is only raised when using :

    -d, --remove-dups                   Exclude from statistics reads marked as duplicates

    -f, --required-flag  <str|int>      Required flag, 0 for unset. See also `samtools flags` [0]
    -F, --filtering-flag <str|int>      Filtering flag, 0 for unset. See also `samtools flags` [0]

the reads are then filtered = ignored from the other counts. If no user uses those flags, the filtered count will always be 0 = no bug

Are there any plots / tables that should include this as a category as well?

🤷

sorry, I'm away from my lab, I don't know if the current MQC table displays the number of filtered reads.

@vladsavelyev
Copy link
Member

@lindenb - thanks a lot for the contribution, it looks good to me, but would you be able to submit a test example that reproduces the bug into MultiQC_TestData? Thank you!

@vladsavelyev vladsavelyev changed the title fix bug when counting reads in samtools stats samtools-stats: fix "Percent Mapped" plot when samtools was run with read filtering Aug 23, 2023
@vladsavelyev vladsavelyev changed the title samtools-stats: fix "Percent Mapped" plot when samtools was run with read filtering samtools-stats: fix "Percent Mapped" plot when tools was run with read filtering Aug 23, 2023
@vladsavelyev vladsavelyev self-requested a review August 23, 2023 10:38
Copy link
Member

@vladsavelyev vladsavelyev left a comment

Choose a reason for hiding this comment

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

Asking for a test example, otherwise good to me

@vladsavelyev vladsavelyev added the waiting: response Waiting for more information from user label Aug 23, 2023
lindenb added a commit to lindenb/MultiQC_TestData that referenced this pull request Aug 23, 2023
@lindenb
Copy link
Contributor Author

lindenb commented Aug 23, 2023

@lindenb - thanks a lot for the contribution, it looks good to me, but would you be able to submit a test example that reproduces the bug into MultiQC_TestData? Thank you!

@vladsavelyev thanks for the review & done : MultiQC/test-data#267

Copy link
Member

@vladsavelyev vladsavelyev left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@vladsavelyev vladsavelyev added awaits-review Awaiting final review and merge. and removed waiting: response Waiting for more information from user labels Aug 24, 2023
@ewels ewels changed the title samtools-stats: fix "Percent Mapped" plot when tools was run with read filtering Samtools: Stats: fix "Percent Mapped" plot when samtools was run with read filtering Aug 24, 2023
@ewels
Copy link
Member

ewels commented Aug 24, 2023

Thanks @lindenb!

@ewels ewels merged commit 965fbc0 into MultiQC:master Aug 24, 2023
16 checks passed
@lindenb lindenb deleted the pl_samtools_stats branch August 24, 2023 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaits-review Awaiting final review and merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants