Add CIGAR string support to alignment IO #1192

smehringer · 2019-07-23T06:59:42Z

~~Blocked by #1194~~ see last commit, the alternative way to get the help page description does not cause the ICE, so I am not dependent on the ranges-v3 update.

Review commit by commit and note the following:

regarding commit 1: When the alingment IO was written, no cigar alphabet was present. This commit introduces the alphabet and replaces pair<char, uint32_t>.
regarding commit 2: This commit belongs to the refactoring of the first, but it did ONLY move the get_cigar_vector code above the get_cigar_string code to make use of it. Git does a horrible job in detecting this change (where actually there is none...). So I kept it separate to see the actual differences in the former commit.
regarding commit 3: We ignored hard clipping and stored soft clipping in a separate variable before because it was not needed. Now the CIGAR string should represent the exact input, with hard clipping, so the functions were adapted to return the complete cigar vector.
regarding commit 4: The parse_cigar/parse_binary_cigar functions were format specific and should not be free functions but member functions.
regarding commit 5: Support reading of CIGAR fields.
regarding commit 6: Support writing of CIGAR fields. Note that for BAM, more elaborate changes were needed to compute the ref_length from the CIGAR string (formerly obtained by simply querying the alignment length) because BAM writes out bin information for the BAM index that needs the ref_length information.

joergi-w

For commits 1–4. It's mostly about documentation that was not adapted to code changes.

include/seqan3/io/alignment_file/detail.hpp

include/seqan3/io/alignment_file/format_sam.hpp

include/seqan3/io/alignment_file/format_bam.hpp

include/seqan3/io/alignment_file/format_sam.hpp

joergi-w

For commits 5-6.

doc/tutorial/alignment_file/alignment_file_read_cigar.cpp

doc/tutorial/alignment_file/index.md

doc/tutorial/alignment_file/alignment_file_read_cigar.cpp

include/seqan3/io/alignment_file/input.hpp

test/unit/io/alignment_file/alignment_file_format_test_template.hpp

include/seqan3/io/alignment_file/detail.hpp

include/seqan3/io/alignment_file/format_bam.hpp

include/seqan3/io/alignment_file/format_sam.hpp

test/unit/io/alignment_file/alignment_file_format_test_template.hpp

joergi-w

One more curiosity...

include/seqan3/io/alignment_file/output_format_concept.hpp

joergi-w

This is already really good! However, a few small issues remain:

doc/tutorial/alignment_file/index.md

include/seqan3/io/alignment_file/detail.hpp

include/seqan3/io/alignment_file/format_bam.hpp

include/seqan3/io/alignment_file/format_sam.hpp

joergi-w

Also please check the CI results (header test + doxygen).

include/seqan3/io/alignment_file/input_format_concept.hpp

codecov · 2019-07-26T10:31:56Z

Codecov Report

Merging #1192 into master will decrease coverage by 0.01%.
The diff coverage is 97.72%.

@@            Coverage Diff            @@
##           master   #1192      +/-   ##
=========================================
- Coverage   97.61%   97.6%   -0.02%     
=========================================
  Files         222     222              
  Lines        9007    8961      -46     
=========================================
- Hits         8792    8746      -46     
  Misses        215     215

Impacted Files	Coverage Δ
include/seqan3/io/alignment_file/input.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/io/alignment_file/output.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/io/record.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/io/alignment_file/format_bam.hpp	`95.73% <100%> (+0.33%)`	⬆️
include/seqan3/io/sequence_file/format_sam.hpp	`87.5% <100%> (ø)`	⬆️
include/seqan3/io/alignment_file/format_sam.hpp	`98.6% <95.55%> (-0.38%)`	⬇️
include/seqan3/io/alignment_file/detail.hpp	`96.77% <97.36%> (-0.92%)`	⬇️

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 19ab4cd...1cf7756. Read the comment docs.

joergi-w

Thank you, looks good!

ppericard · 2019-09-03T09:00:35Z

Hi,
Any news about integrating this pull request?
I am very eager to be able to get the CIGAR string from alignment files to continue my developments.
Thanks for all the work ^_^
Pierre

smehringer · 2019-09-04T08:45:24Z

Hi @ppericard,

great to hear that this module is in use :)

There is an internal compiler error in the range library that has been fixed in the new range-v3 release so I am just wating for some PRs that make our library compatible to the that release. I think it will be a matter of 2-3 weeks.

I will try raise the priority of this!

include/seqan3/io/alignment_file/detail.hpp

doc/tutorial/alignment_file/index.md

include/seqan3/io/alignment_file/format_bam.hpp

include/seqan3/io/alignment_file/output_format_concept.hpp

include/seqan3/io/sequence_file/format_sam.hpp

rrahn

Some minor stuff 💅

include/seqan3/argument_parser/validators.hpp

include/seqan3/io/alignment_file/detail.hpp

include/seqan3/io/alignment_file/format_bam.hpp

include/seqan3/io/alignment_file/format_sam.hpp

include/seqan3/io/alignment_file/output.hpp

rrahn

I think that was the final round.

include/seqan3/io/alignment_file/detail.hpp

rrahn · 2019-09-30T17:12:29Z

include/seqan3/io/alignment_file/detail.hpp

- *            is based on sequence at the second position of the \p alignment pair,
- *            namely the query sequence.
+ * \attention Note that CIGAR elements (respectively by their CIGAR operation) are always related to one of the
+ *            two sequences in a pairwise alignment. In this case, the resulting cigar_vector is based on sequence


I don't understand this. What information do you like to express?

That the cigar string is a relative thing... If it says "deletion of 2 bases", it means a deletion in the query not the reference, so you need to aware which is the reference sequence and which is the query sequence.

In the case of map_aligned_values_to_cigar_op it is query_char and reference_char.

I don't know. I find the description a bit complicated. I proposed some alternative description in a comment above. What do you think?

include/seqan3/io/alignment_file/format_sam.hpp

smehringer · 2019-10-01T05:49:29Z

@rrahn Please mark the others as resolved 🙏

rrahn

Only one little 💅 thing for some documentation.

rrahn · 2019-10-02T11:33:01Z

include/seqan3/io/alignment_file/detail.hpp

 *
- * The following alignment reference sequence on top and the query sequence at
- * the bottom.
+ * \attention Note that CIGAR elements (respectively by their CIGAR operation) are always related to one of the two


Suggested change

* \attention Note that CIGAR elements (respectively by their CIGAR operation) are always related to one of the two

The first sequence is always considered the reference sequence while the second one is considered the query sequence. The cigar operations regarding insertions and deletions are set accordingly.

Is it this, what you want to express?
If yes, could make the same/similar note above for the other function?

rrahn · 2019-10-02T11:35:12Z

include/seqan3/io/alignment_file/detail.hpp

- *            is based on sequence at the second position of the \p alignment pair,
- *            namely the query sequence.
+ * \attention Note that CIGAR elements (respectively by their CIGAR operation) are always related to one of the
+ *            two sequences in a pairwise alignment. In this case, the resulting cigar_vector is based on sequence


I don't know. I find the description a bit complicated. I proposed some alternative description in a comment above. What do you think?

rrahn · 2019-10-02T11:35:41Z

include/seqan3/io/alignment_file/detail.hpp

 *            in a pairwise alignment. In this case, the resulting cigar_vector
 *            is based on sequence at the second position of the \p alignment pair,
 *            namely the query sequence.
 *
- * ### Example:
+ * ### Theoretical Example:


unresolved?

rrahn · 2019-10-02T11:36:29Z

include/seqan3/io/alignment_file/detail.hpp

- *            in a pairwise alignment. In this case, the resulting cigar_vector
- *            is based on sequence at the second position of the \p alignment pair,
- *            namely the query sequence.
+ * \attention Note that CIGAR elements (respectively by their CIGAR operation) are always related to one of the two


that would be also a candidate not to miss out if you agree on changing the description as proposed.

rrahn

documentation stuff 💅

rrahn · 2019-10-07T16:54:04Z

include/seqan3/io/alignment_file/detail.hpp

+ * \tparam alignment_type  Must model the seqan3::tuple_like and must have std::tuple_size 2.
+ *                         Each tuple element must be a range over values comparable to seqan3::gap.
+ * \param  alignment       The alignment, represented by a pair of aligned sequences,
+ *                         to be transformed into CIGAR_vector based on the


CIGAR_vector cigar_vector cigar vector

From these three I would prefer the latter since it does not name a variable really. Also in the note block

rrahn

I am truly sorry, I just saw that there is still inconsistent uses of CIGAR string and cigar string as well. It is ok to leave it as is, but if you want to change it please keep me a heads up. Otherwise I will just merge it in a 30 minutes or so.

smehringer · 2019-10-08T12:16:47Z

@rrahn I also updated the changelog!

…nore clipping when constructing the alignment.

…d reduce redundant code.

smehringer · 2019-10-09T08:25:20Z

@rrahn I think this can be merged now. I also rebased on current master.

smehringer · 2019-10-09T14:05:28Z

@rrahn ping

smehringer force-pushed the alignmentIO_cigar_string branch 2 times, most recently from 09edc53 to 956eff3 Compare July 23, 2019 07:38

smehringer requested a review from marehr July 23, 2019 07:39

smehringer force-pushed the alignmentIO_cigar_string branch from 956eff3 to bc4c356 Compare July 23, 2019 11:17

smehringer requested review from joergi-w and removed request for marehr July 24, 2019 11:06

joergi-w requested changes Jul 24, 2019

View reviewed changes

joergi-w requested changes Jul 25, 2019

View reviewed changes

include/seqan3/io/alignment_file/output_format_concept.hpp Show resolved Hide resolved

smehringer force-pushed the alignmentIO_cigar_string branch from bc4c356 to 3887b07 Compare July 25, 2019 13:49

smehringer requested a review from joergi-w July 25, 2019 13:51

joergi-w requested changes Jul 26, 2019

View reviewed changes

include/seqan3/io/alignment_file/input_format_concept.hpp Outdated Show resolved Hide resolved

smehringer force-pushed the alignmentIO_cigar_string branch from 3887b07 to cd42682 Compare July 26, 2019 10:31

joergi-w approved these changes Jul 26, 2019

View reviewed changes

smehringer force-pushed the alignmentIO_cigar_string branch 2 times, most recently from db2f70d to 3bc5b0a Compare September 16, 2019 13:17

smehringer requested a review from rrahn September 17, 2019 06:53

rrahn requested changes Sep 24, 2019

View reviewed changes

smehringer force-pushed the alignmentIO_cigar_string branch 2 times, most recently from 2520271 to da4e5b1 Compare September 25, 2019 10:29

smehringer requested a review from rrahn September 25, 2019 10:29

rrahn requested changes Sep 26, 2019

View reviewed changes

smehringer force-pushed the alignmentIO_cigar_string branch from da4e5b1 to c2c7f62 Compare September 30, 2019 14:27

smehringer requested a review from rrahn September 30, 2019 14:46

rrahn requested changes Sep 30, 2019

View reviewed changes

smehringer force-pushed the alignmentIO_cigar_string branch from c2c7f62 to 0de953e Compare October 1, 2019 06:01

smehringer requested a review from rrahn October 1, 2019 12:49

rrahn requested changes Oct 2, 2019

View reviewed changes

smehringer force-pushed the alignmentIO_cigar_string branch 2 times, most recently from dada220 to 2062407 Compare October 7, 2019 13:47

smehringer requested a review from rrahn October 7, 2019 13:47

rrahn requested changes Oct 7, 2019

View reviewed changes

smehringer force-pushed the alignmentIO_cigar_string branch from 2062407 to 2abd6eb Compare October 8, 2019 07:14

smehringer requested a review from rrahn October 8, 2019 07:14

rrahn approved these changes Oct 8, 2019

View reviewed changes

smehringer force-pushed the alignmentIO_cigar_string branch 2 times, most recently from fabc4bb to 654bb1c Compare October 8, 2019 12:16

smehringer and others added 7 commits October 9, 2019 08:02

[MISC] Adapt vector of pairs to vector of cigar elements.

384307c

[MISC] make get_cigar_string delegate to get_cigar_vector.

35674d1

[MISC] Don't strip the cigar string of softclipping beforehand but ig…

9dbfc65

…nore clipping when constructing the alignment.

[MISC] Make format specific detail function into a member function an…

1b50cb8

…d reduce redundant code.

[FEATURE] SAM format can now read the cigar string if specified.

127a18d

[FEATURE] SAM/BAM can now write a cigar vector instead of an alignment.

60c8ad6

[FIX] Add format_bam.hpp to alignment_file/all.hpp

1cf7756

smehringer force-pushed the alignmentIO_cigar_string branch from 654bb1c to 1cf7756 Compare October 9, 2019 06:24

rrahn merged commit 9f98962 into seqan:master Oct 9, 2019

smehringer deleted the alignmentIO_cigar_string branch May 29, 2020 05:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CIGAR string support to alignment IO #1192

Add CIGAR string support to alignment IO #1192

smehringer commented Jul 23, 2019 •

edited

Loading

joergi-w left a comment

joergi-w left a comment

joergi-w left a comment

joergi-w left a comment

joergi-w left a comment

codecov bot commented Jul 26, 2019 •

edited

Loading

joergi-w left a comment

ppericard commented Sep 3, 2019

smehringer commented Sep 4, 2019

rrahn left a comment

rrahn left a comment

rrahn Sep 30, 2019

smehringer Oct 1, 2019

rrahn Oct 2, 2019

smehringer commented Oct 1, 2019

rrahn left a comment

rrahn Oct 2, 2019

rrahn Oct 2, 2019

rrahn Oct 2, 2019

rrahn Oct 2, 2019

rrahn left a comment

rrahn Oct 7, 2019

rrahn left a comment

smehringer commented Oct 8, 2019

smehringer commented Oct 9, 2019

smehringer commented Oct 9, 2019

	* \attention Note that CIGAR elements (respectively by their CIGAR operation) are always related to one of the two
	The first sequence is always considered the reference sequence while the second one is considered the query sequence. The cigar operations regarding insertions and deletions are set accordingly.

Add CIGAR string support to alignment IO #1192

Add CIGAR string support to alignment IO #1192

Conversation

smehringer commented Jul 23, 2019 • edited Loading

joergi-w left a comment

Choose a reason for hiding this comment

joergi-w left a comment

Choose a reason for hiding this comment

joergi-w left a comment

Choose a reason for hiding this comment

joergi-w left a comment

Choose a reason for hiding this comment

joergi-w left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 26, 2019 • edited Loading

Codecov Report

joergi-w left a comment

Choose a reason for hiding this comment

ppericard commented Sep 3, 2019

smehringer commented Sep 4, 2019

rrahn left a comment

Choose a reason for hiding this comment

rrahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smehringer commented Oct 1, 2019

rrahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrahn left a comment

Choose a reason for hiding this comment

smehringer commented Oct 8, 2019

smehringer commented Oct 9, 2019

smehringer commented Oct 9, 2019

smehringer commented Jul 23, 2019 •

edited

Loading

codecov bot commented Jul 26, 2019 •

edited

Loading