Added LUT for merged header in MultipleVariantReader

broadinstitute / gamgee

A C++14 library for NGS data formats

http://broadinstitute.github.io/gamgee/

MIT License

40 stars 13 forks source link

Added LUT for merged header in MultipleVariantReader #376

Closed kgururaj closed 9 years ago

kgururaj commented 9 years ago

Added MergedVCFLUT capability in utils/merged_vcf_utils.*. This class is now used within MultipleVariantReader.
Added test case for LUT.
Fixed signed-unsigned comparisons
Added some useful functions in VariantHeader

TODO:

What should be done when sample names are duplicated?

coveralls commented 9 years ago

Coverage decreased (-1.21%) when pulling 5c89c1f70428ca8a0c656f8ab9a6f872de7d7755 on intel_var_header_merger into 036af7078adb5625b9ea7ef072ca5da9c1d07920 on master.

droazen commented 9 years ago

First pass review complete -- gave high-level feedback only for now. Will do a more detailed pass over the code once the high-level questions are addressed.

kgururaj commented 9 years ago

Edited the code based on your feedback

coveralls commented 9 years ago

Coverage increased (+8.13%) when pulling 3952a967d1a86149b76f6ff9ef14aa15167789a1 on intel_var_header_merger into 05676cf5909257068ca8cdc5cdb26d61cc60d5b5 on master.

droazen commented 9 years ago

Second-pass review complete

coveralls commented 9 years ago

Coverage increased (+6.26%) when pulling ebbbf899fe51d270ffe400ea3d2d84f29b15d190 on intel_var_header_merger into 05676cf5909257068ca8cdc5cdb26d61cc60d5b5 on master.

kgururaj commented 9 years ago

Edits done

coveralls commented 9 years ago

Coverage increased (+3.83%) when pulling 1169cd4a2c2ac6dc0236c7d4c2754a2a800de449 on intel_var_header_merger into 05676cf5909257068ca8cdc5cdb26d61cc60d5b5 on master.

jmthibault79 commented 9 years ago

The latest changes look great, thanks!

I am ready for you to rebase (on a post #382 master) and squash this to a single commit, but I would like @droazen to weigh in as well.

Note that there have been conflicting changes in variant_builder_test.cpp since this PR, so be aware of that when rebasing.

droazen commented 9 years ago

Looks really great Karthik -- as far as I can tell all issues raised in the last code review have been addressed. I had two more (quick) requests this time around related to get_merged_header() and copy/move semantics of the VariantHeaderMerger class. Once these are addressed, this branch can be rebased/merged.

droazen commented 9 years ago

Whoops, one more thing I forgot to mention: please add all new files to gamgee.h

kgururaj commented 9 years ago

Edits done - should I 'compress' all changes to 1 commit?

droazen commented 9 years ago

:+1: Looks good Karthik -- feel free to rebase/squash and merge!

coveralls commented 9 years ago

Coverage increased (+4.95%) when pulling 54828da0969f0c8d10eda74143311c17ce7cb22f on intel_var_header_merger into a25b637cdba32d3ade0dafc9f5f76ee7b443e051 on master.

coveralls commented 9 years ago

Coverage increased (+10.37%) when pulling e9d13b75b19a20ae5107ab6be979286365a8c1c8 on intel_var_header_merger into a25b637cdba32d3ade0dafc9f5f76ee7b443e051 on master.