Bam dialog matches #795

Merged
merged 32 commits into from Mar 22, 2017

Conversation

Projects
None yet
3 participants
@cmdcolin
Contributor

cmdcolin commented Aug 1, 2016

This is a PR to address the feature request in #496

The algorithm is able to reconstruct the alignment from the BAM read sequence+MD tag+CIGAR string

I created some test cases to handle some common scenarios addressed by the algorithm (insertion, deletion, soft clipping reads)

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Aug 4, 2016

Contributor

we can compare the output of sam2pairwise as a reference for this too. https://github.com/mlafave/sam2pairwise

it seems to match the output as expected

Contributor

cmdcolin commented Aug 4, 2016

we can compare the output of sam2pairwise as a reference for this too. https://github.com/mlafave/sam2pairwise

it seems to match the output as expected

@keiranmraine

This comment has been minimized.

Show comment
Hide comment
@keiranmraine

keiranmraine Aug 4, 2016

Contributor

@cmdcolin , the only other thing I'd like to see is (if possible) wrapping the display like GBrowse does, e.g.

screen shot 2016-08-04 at 09 03 12

Contributor

keiranmraine commented Aug 4, 2016

@cmdcolin , the only other thing I'd like to see is (if possible) wrapping the display like GBrowse does, e.g.

screen shot 2016-08-04 at 09 03 12

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Aug 5, 2016

Contributor

@keiranmraine Added that "newline based" rendering behind a config flag (renderAlignment.newlines). Width can be configured as renderAlignment.width

Contributor

cmdcolin commented Aug 5, 2016

@keiranmraine Added that "newline based" rendering behind a config flag (renderAlignment.newlines). Width can be configured as renderAlignment.width

@keiranmraine

This comment has been minimized.

Show comment
Hide comment
@keiranmraine

keiranmraine Aug 6, 2016

Contributor

@cmdcolin, that works well thanks!

Contributor

keiranmraine commented Aug 6, 2016

@cmdcolin, that works well thanks!

@keiranmraine

This comment has been minimized.

Show comment
Hide comment
@keiranmraine

keiranmraine Sep 5, 2016

Contributor

Any ETA on a merge for this?

Contributor

keiranmraine commented Sep 5, 2016

Any ETA on a merge for this?

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Mar 22, 2017

Contributor

I made this feature optional and off by default (i.e. introduce the feature behind a feature flag) so would maybe be safe to merge and interested users can enable it if interested.

Contributor

cmdcolin commented Mar 22, 2017

I made this feature optional and off by default (i.e. introduce the feature behind a feature flag) so would maybe be safe to merge and interested users can enable it if interested.

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Mar 22, 2017

Contributor

Given that this is behind a feature flag, would it be ok to merge this one?

I just know it was requested by @keiranmraine and having something like this available can be pretty useful when inspecting alignments

Contributor

cmdcolin commented Mar 22, 2017

Given that this is behind a feature flag, would it be ok to merge this one?

I just know it was requested by @keiranmraine and having something like this available can be pretty useful when inspecting alignments

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Mar 22, 2017

Contributor

some screenshots:

normal short read
screenshot-localhost-2017-03-22-17-03-58 1

long read
screenshot-localhost-2017-03-22-17-10-46 1

rna-seq read
screenshot-localhost-2017-03-22-17-11-02 1

note that the alignment is constructed entirely from the MD tag and CIGAR string so it doesn't need to retreive reference sequence track data

Contributor

cmdcolin commented Mar 22, 2017

some screenshots:

normal short read
screenshot-localhost-2017-03-22-17-03-58 1

long read
screenshot-localhost-2017-03-22-17-10-46 1

rna-seq read
screenshot-localhost-2017-03-22-17-11-02 1

note that the alignment is constructed entirely from the MD tag and CIGAR string so it doesn't need to retreive reference sequence track data

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Mar 22, 2017

Contributor

I think we're gonna try and get this merged behind feature flag! that way it can get tested and improved :)

Contributor

cmdcolin commented Mar 22, 2017

I think we're gonna try and get this merged behind feature flag! that way it can get tested and improved :)

@cmdcolin cmdcolin merged commit 4fd08b5 into master Mar 22, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@cmdcolin cmdcolin deleted the bam_dialog_matches branch Mar 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment