Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Negative strand option for liftover #4895

Merged
merged 5 commits into from Dec 6, 2018
Merged

Conversation

@jigold
Copy link
Collaborator

@jigold jigold commented Dec 4, 2018

@konradjk @bw2 Can you double check the test examples in test_liftover_negative_strand and give feedback on the interface?

Copy link
Collaborator

@konradjk konradjk left a comment

Did you all decide that mixed return types depending on parameters was ok? I'm fine with it, but I thought there was hesitation. In any case, interface looks fine to me. I'll let @bw2 comment on the exact examples

If True, output the result as a :class:`.StructExpression` with the first field `result` being
the locus or locus interval and the second field `is_negative_strand` is a boolean indicating
whether the locus or locus interval has been mapped to the negative strand of the destination
reference genome.
Copy link
Collaborator

@konradjk konradjk Dec 5, 2018

Needs an Otherwise, ...

@bw2
Copy link
Contributor

@bw2 bw2 commented Dec 5, 2018

At first glance, the tests look fine to me. More thorough tests could be done using the data files here
https://github.com/broadinstitute/picard/blob/e0bb690d57f73fd2495fc5a77b497e9696f51f81/src/test/java/picard/util/LiftoverVcfTest.java#L65-L99

The interface also looks fine in terms of a non-breaking way to add strand info.
Is the plan to use this to implement a hl.liftover(mt) function that includes the checks from https://github.com/broadinstitute/picard/blob/master/src/main/java/picard/vcf/LiftoverVcf.java#L350-L397 ?

@jigold
Copy link
Collaborator Author

@jigold jigold commented Dec 5, 2018

Thanks! I'll look into this later. The plan is to have a liftover_rows method that does the same thing as LiftoverVCF. However, this requires infrastructure to do a while loop, which needs to be built first.

@bw2
Copy link
Contributor

@bw2 bw2 commented Dec 6, 2018

oh, interesting.. I thought liftover processed each variant independently. Just out of curiosity, what's the purpose of the special while-loop infrastructure?
and will it be possible to avoid shuffling when sorting lifted-over variants?

@jigold
Copy link
Collaborator Author

@jigold jigold commented Dec 6, 2018

The liftover function is just a mapping from old locus to new locus. The method I'm thinking of implementing will do the left aligning and strand flipping of the alleles as well. I haven't decided if we should be trying to flip the genotypes at all or some of the more complicated things that function does. The while loop is needed to left align the indels in the case the strand flipped. See this code: https://github.com/broadinstitute/picard/blob/dee16518d89838fac40c6f9c07f342430326cb16/src/main/java/picard/util/LiftoverUtils.java#L365-L428

@jigold
Copy link
Collaborator Author

@jigold jigold commented Dec 6, 2018

I looked at the picard tests and realized they just made a fake chain file with one interval that mapped to the negative strand in the destination reference. My existing test covers this scenario -- one interval maps to the positive strand of the destination and the other to the negative strand. Since I'm only passing the strand information from Picard, I'm happy with the tests I have already.

@konradjk
Copy link
Collaborator

@konradjk konradjk commented Dec 6, 2018

I trust that it's fine then (and if it's not, I'm sure we'll come by to complain at that point 😄). @patrick-schultz over to you

@danking danking merged commit 4a9ff37 into hail-is:master Dec 6, 2018
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants