Unify Giraffe and vg surject surjection - #5025
Merged
Merged
Conversation
`vg giraffe --output-format bam` can make properly paired output, and this is under test https://github.com/vgteam/vg/blob/fd49b9a9040716197f3b23521e14ca29e547fbb7/test/t/50_vg_giraffe.t#L204-L209 `vg surject` can make properly paired output from `vg map`'s GAMs, and this is under test https://github.com/vgteam/vg/blob/fd49b9a9040716197f3b23521e14ca29e547fbb7/test/t/15_vg_surject.t#L117-L124 However, if you do `vg giraffe` to get a GAM, and then do `vg surject` on that GAM, the resulting BAM output will not be properly paired. This is documented in #4563 and I can easily reproduce the behavior, though I have no idea why it's happening. Here I've just made a unit test which fails.
though honestly it should auto-detect
…ary surjection handling together so they can't disagree
…guess secondary assignments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog Entry
To be copied to the draft changelog by merger:
vg giraffeandvg surjectnow surject paired-end reads with the same code.Description
This fixes #4563 by making
vg surjectand theSurjectingAlignmentEmitterused byvg giraffedo all their pair surjection with the same function, instead of two implementations with two different notions of how to set up supplementaries or match up surjection results. I'm stealing it off ofSurjectingAlignmentEmitterand putting it onSurjector.I'm also running paired-end multipath surjection through this function, since it had yet a third implementation of this. This makes the function really complex, but it does ensure that we can't end up doing things weirdly differently depending on alignment format.
The behavior of all 3 modes is going to change. The new behavior is probably more right (it finds paths and strands that we have both reads on, picks the best scoring multi-surjection on each path strand, and then the path strand with the best 2 of those), but it's still not really right because it doesn't e.g. match up surjections according to fragment length, and when we don't have a pair on the same path and strand we just pick the first result for each read.
@Sagorikanag has code that should obviate almost all of this, and properly handle the notion of a surjection of a pair, but I can't merge it yet.