Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add custom equality function as walk_together argument #132

Merged
merged 1 commit into from Dec 3, 2013

Conversation

Projects
None yet
3 participants

bow commented Dec 3, 2013

The walk_together function is very useful for iterating over multiple VCF files. However, it breaks in some cases (e.g. comparing multiple single - sample VCF files with one containing reference calls) due to its definition of VCF record equality (which sometimes may be too strict).

This pull request adds the ability to use user-defined equality functions (e.g. if we want to compare only on Record positions and reference bases), while still leaving the original behavior intact. I should mention that this comes at a cost of a rather clunky usage of **kwargs in walk_together, but I think this is an acceptable trade off.

New test cases against this change have also been added and feedbacks are very welcomed :).

Owner

jamescasbon commented Dec 3, 2013

This looks good to me, @martijnvermaat any thoughts?

Collaborator

martijnvermaat commented Dec 3, 2013

I think this is good to go, thanks for the PR @bow!

@martijnvermaat martijnvermaat added a commit that referenced this pull request Dec 3, 2013

@martijnvermaat martijnvermaat Merge pull request #132 from bow/patch_walk-customfunc
Add custom equality function as walk_together argument
cede181

@martijnvermaat martijnvermaat merged commit cede181 into jamescasbon:master Dec 3, 2013

bow commented Dec 3, 2013

Thank you @jamescasbon & @martijnvermaat for merging :).

Slightly related to this, I've been thinking about the behavior of _Record.__eq__ (https://github.com/jamescasbon/PyVCF/blob/master/vcf/model.py#L151), actually. Perhaps it's a good idea to have it always return False if any one of the operand is None? That way we can avoid the TypeError that's always raised whenever you compare to None. I could open a new issue / PR for this if that's preferred (or use this one along with the issue below).

Also, a note on the failing build. I've overlooked the fact that Python2.6 doesn't have self.assertRaisesRegexp which I use in one of the tests. I can remove that bit and use just self.assertRaises instead.

Collaborator

martijnvermaat commented Dec 4, 2013

Apparently some of the unit tests were never run on Travis (including this one), which is why I didn't notice the fialure on Python 2.6. I fixed this in 322c212 and b8c0af7 (although some of the unittest mechanics are really beyond me I have to admit).

This was also a nice opportunity for me to try the Tox tool. It works pretty well.

Could you open a new issue or pull request for changing _Record.__eq__?

bow commented Dec 4, 2013

Great, thanks for the test fix :). I'll open a new PR for the __eq__ change soon.

@gotgenes gotgenes pushed a commit to gotgenes/PyVCF that referenced this pull request May 13, 2014

@martijnvermaat martijnvermaat Merge pull request #132 from bow/patch_walk-customfunc
Add custom equality function as walk_together argument
cf5d0c5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment