Skip to content

Leave low-scoring MAPQ 0 reads unmapped#4916

Merged
adamnovak merged 10 commits into
masterfrom
mapq0-score
Jun 2, 2026
Merged

Leave low-scoring MAPQ 0 reads unmapped#4916
adamnovak merged 10 commits into
masterfrom
mapq0-score

Conversation

@faithokamoto
Copy link
Copy Markdown
Contributor

Changelog Entry

To be copied to the draft changelog by merger:

  • Add new vg giraffe filter for low-scoring MAPQ 0 R10 reads

Description

Per Slack discussion about how ridiculously low scoring MAPQ 0 R10 reads are invariably mapped incorrectly, here is my attempt at an implementation. It managed to successfully not align a test read. Basically this gives us a way to refuse to use an alignment if it looks terrible and we have no confidence in it. It's only on for R10 mode.

@faithokamoto
Copy link
Copy Markdown
Contributor Author

Yeah this is currently borked for track provenance stuff, sigh. I am trying to divine what magic the funnel system wants from me.

@faithokamoto
Copy link
Copy Markdown
Contributor Author

@adamnovak one of the tests is failing with

Exception: Error running rtg tools. Return code was 1, output:  / Cannot determine system memory!Could not automatically choose percentage based memory allocation, check configuration. Using Java default.nullError: RTG has encountered a difficulty, please contact support@realtimegenomics.com

and I don't think that's my fault?

Copy link
Copy Markdown
Member

@adamnovak adamnovak left a comment

Choose a reason for hiding this comment

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

I think the option documentation is wrong, and I think the funnel stuff would work a lot more neatly if we declared that this post-filtering is happening in a new funnel stage.

I'm not sure what's wrong with the CI tests; something about the new CI runners seems to have broken test job 7 everywhere. I will see if I can fix that somehow.

Comment thread src/subcommand/giraffe_main.cpp Outdated
"min-mapq0-score",
&MinimizerMapper::min_mapq0_score,
MinimizerMapper::default_min_mapq0_score,
"refuse to align lower scoring MQ 60 reads"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nope that's not right.

It might make more sense to explain this as discarding the alignments or leaving the reads unaligned, rather than refusing to align them, because by the time we know the score we've computed the alignment.

Comment thread src/minimizer_mapper_from_chains.cpp
Copy link
Copy Markdown
Member

@adamnovak adamnovak left a comment

Choose a reason for hiding this comment

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

I think the funnel setup wasn't explained enough, so this still isn't quite right.

I'm not having good luck in #4918 so we might need to merge this over the failure of that CI test (or maybe just xfail it until it can be fixed).

Comment thread src/minimizer_mapper_from_chains.cpp Outdated
if (track_provenance) {
funnel.stage("demapping");
for (size_t i = 0; i < mappings.size(); i++) {
funnel.project(i);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We're meant to figure out if the item at the previous stage fails any filter, or if it passes all filters, and then only project to an item at this stage if it passed all the filters. See:

vg/src/funnel.hpp

Lines 139 to 140 in 21c5309

/// Fail the given item from the previous stage on the given filter and do not project it through to this stage.
/// Items which do not fail a filter must pass the filter and be projected to something.

So the project call should probably be right after the pass() call, instead of here.

@faithokamoto
Copy link
Copy Markdown
Contributor Author

@adamnovak ERROR: failed to push quay.io/vgteam/vg:cache-mapq0-score-build: denied: System is currently read-only. Pulls will succeed but all write operations are currently suspended.

@adamnovak
Copy link
Copy Markdown
Member

Quay has returned, apparently.

@adamnovak adamnovak merged commit cee90d2 into master Jun 2, 2026
2 checks passed
@faithokamoto faithokamoto deleted the mapq0-score branch June 2, 2026 21:19
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.

2 participants