Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Conversation

jmthibault79
Copy link
Contributor

Enables iteration over multiple input files, returning a vector of Variant objects per genomic location.

The vector is reused per location, so the caller must complete processing before continuing iteration, or make a copy.

Strongly inspired by the example of VariantReader/VariantIterator. Merging these Readers or Iterators may be appropriate.

This is an alternative solution to Issue #76

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably better here, but we should have a flag that bypasses this validation (most of the time we will be merging files that were generated by the same tool, thus not needing the time consuming validation).

@MauricioCarneiro
Copy link
Contributor

done reviewing! Great job @jmthibault79 this looks really good. I want @droazen to also take a look. With these important parts of the engine we should be very minutious with the review.

@jmthibault79
Copy link
Contributor Author

Thanks.

These small commits address your comments except:

  • header merging, validation, and bypass flag
  • hacky MultipleVariantIterator::operator!=

Travis is still failing, and I'm having trouble parsing the error messages. It would help if we had access to an environment similar to Travis'.

@MauricioCarneiro
Copy link
Contributor

I think your problem is in this line:

std::vector<const std::shared_ptr<bcf_hdr_t>> m_variant_headers; ///< vector of the internal header structures of the variant files

you can't make the std::shared_ptr const because you wouldn't be able to change the reference counting and other things. This is throwing the entire STD library off. See from this error message in Travis:

gamgee/multiple_variant_reader.h:106:49: note: in instantiation of template class 'std::vector<const std::shared_ptr<bcf_hdr_t>, std::allocator<const std::shared_ptr<bcf_hdr_t> > >' requested here
  std::vector<const std::shared_ptr<bcf_hdr_t>> m_variant_headers;  ///< vector of the internal header structures of the variant files

also note from this error line how the typecasting from const shared_ptr to void * is impossible because you would lose constness:

/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_construct.h:75:13: error: static_cast from 'const std::shared_ptr<bcf_hdr_t> *' to 'void *' is not allowed

@jmthibault79
Copy link
Contributor Author

Thanks for the Travis help. That makes sense. When I change that structure to hold only one header, I'll make sure that it's not const.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.8%) when pulling 9d1a79b on jt_pqueue into cdafada on master.

@MauricioCarneiro
Copy link
Contributor

woot woot ! we broke the 70% barrier! 👍

@coveralls
Copy link

Coverage Status

Coverage increased (+1.68%) when pulling 22fa144 on jt_pqueue into fe67107 on master.

@MauricioCarneiro
Copy link
Contributor

has my blessing, next step benchmark it against bcftools!

MauricioCarneiro pushed a commit that referenced this pull request Jul 11, 2014
MultipleVariantReader and MultipleVariantIterator
@MauricioCarneiro MauricioCarneiro merged commit 9ba5887 into master Jul 11, 2014
@MauricioCarneiro MauricioCarneiro deleted the jt_pqueue branch July 11, 2014 20:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants