add conversion from SAM to BAM

[jonathanBieler]

Types of changes

This PR implements the following changes:
(Please tick any or all of the following that are applicable)

• ✨ New feature (A non-breaking change which adds functionality).
• 🐛 Bug fix (A non-breaking change, which fixes an issue).
• 💥 Breaking change (fix or feature that would cause existing functionality to change).

📋 Additional detail

This adds a method to convert to convert a SAM record to a BAM record, it will allow to use BWA as a library, create reads in-silico, etc. I'm testing it extensively so it should mostly be correct, but there's still some corner cases I think. I'll add some documentation later. Fix #61

sam = "seq1\t81\tPhiX\t1051\t60\t70M\t=\t1821\t702\tTCTTGGCTTCCTTGCTGGTCAGATTGGTCGTCTTATTACCATTTCAACTACTCCGGTTATCGCTGGCGAC\t*\tNM:i:0\tMD:Z:70\tMC:Z:70M\tAS:i:70\tXS:i:0"
record = XAM.SAM.Record(sam)

bam_record = convert(BAM.Record, record)

☑️ Checklist

• 🎨 The changes implemented is consistent with the julia style guide.
• 📘 I have updated and added relevant docstrings, in a manner consistent with the documentation styleguide.
• 📘 I have added or updated relevant user and developer manuals/documentation in docs/src/.
• 🆗 There are unit tests that cover the code changes I have made.
• 🆗 The unit tests cover my code changes AND they pass.
• 📝 I have added an entry to the [UNRELEASED] section of the manually curated CHANGELOG.md file for this repository.
• 🆗 All changes should be compatible with the latest stable version of Julia.
• 💭 I have commented liberally for any complex pieces of internal code.

[kescobo]

This looks good to me on a first pass, and I really appreciate all the tests! I'm not so familiar with these record types, so I definitely want someone else to take a look, but a few other things to consider in addition to my in-line comments:

1. Do any of these functions make sense to be exported? Eg parsing and encoding CIGAR seem like they could have broader utility.
2. Does this really need to go in its own module?

[kescobo]

Can we add something with an unsupported tag and @test_throws?

[jelber2]

Super cool, I just used this used functionality @jonathanBieler to go from SAM to BAM! see https://gist.github.com/jelber2/47129820373474a768dacabc0e686fdc

[kescobo]

Great! Would be nice to get it merged in so it's a bit more accessible. @jakobnissen or @CiaranOMara - have you had a chance to take a looks?

[jakobnissen]

Overall, looks good, with a few comments.
The code allocates a lot, and I think it needs an API to convert a lot of records in bulk, sharing the same header. This might limit this particular implementation.
However, the API is the most important part. It can always be optimised later.

[jakobnissen]

One more comment, @jonathanBieler - I believe it might be better if you implemented SAM.Record(::BAM.Record) instead of a method of Base.convert. The reason is that convert can be called implicitly, without the programmer's knowledge. This can lead to some problems when conversions happen unexpectedly - implicit copies, degraded performance and more.

[jonathanBieler]

I think I've addressed most of the issues. About performance I think the most common use case will be to write BAM records to a file, so maybe it would be possible to add a write method to BAM.Writer to write directly SAM.Record's and avoid allocations.

[kescobo]

I like that plan, but agree with @jakobnissen that getting the API right is the most important part of this PR. Performance can be fixed later, I think.

@jakobnissen I'm going to let you pull the trigger on merge, one other thing that could be included here is bumping to v0.4.1 so that we can tag a release as soon as it's in.

[CiaranOMara]

Bump the version on a release branch.

[kescobo]

Ah, my bad - I didn't see that's how this repo is organized. Makes sense to me!

[jakobnissen]

Looks good from my PoV (once @CiaranOMara's comment has been addressed - which ever way you prefer)

The Indexes.reg2bins method is not relevant to SAM to BAM conversion.