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

fix issues with #75 #88

Merged
merged 6 commits into from Oct 9, 2017

Conversation

Projects
None yet
3 participants
@gatoravi
Contributor

gatoravi commented Sep 11, 2017

Addresses #75

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 11, 2017

Coverage Status

Coverage increased (+0.1%) to 89.419% when pulling 0290075 on gatoravi:fix_E_v2 into d77dad5 on griffithlab:master.

coveralls commented Sep 11, 2017

Coverage Status

Coverage increased (+0.1%) to 89.419% when pulling 0290075 on gatoravi:fix_E_v2 into d77dad5 on griffithlab:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 11, 2017

Coverage Status

Coverage increased (+0.1%) to 89.419% when pulling 5eea8a9 on gatoravi:fix_E_v2 into d77dad5 on griffithlab:master.

coveralls commented Sep 11, 2017

Coverage Status

Coverage increased (+0.1%) to 89.419% when pulling 5eea8a9 on gatoravi:fix_E_v2 into d77dad5 on griffithlab:master.

@yang-yangfeng

This comment has been minimized.

Show comment
Hide comment
@yang-yangfeng

yang-yangfeng Sep 11, 2017

Contributor

Please correct me if I'm misunderstanding the code, but it looks to me like the new block at 192 will set the cis effect window to ignore the flanking exon blocks (so whereas the splicing_exonic variants will have a window going from [=======]-------[===]----[=====], a splicing_intronic variant will only span -------[===]----, with * representing the variant). Similarly, the windows of intronic variants will span two introns and a central exon (the one before the variant, as in the splicing_intronic "sketch" above).

I believe what we had discussed was to keep exonic, splicing_exonic, and splicing_intronic variants so that their windows would span from the start of the left flanking exon to the end of the right flanking exon (as shown in the splicing_exonic "sketch") while changing the window for intronic variants to only spanning the intron in which the variant resides (or rather, the superset of such introns across transcripts for which the variant is "intronic"). Or am I misremembering? If not, I've proposed some changes in the code to address these concerns.

@malachig, @gatoravi

Contributor

yang-yangfeng commented Sep 11, 2017

Please correct me if I'm misunderstanding the code, but it looks to me like the new block at 192 will set the cis effect window to ignore the flanking exon blocks (so whereas the splicing_exonic variants will have a window going from [=======]-------[===]----[=====], a splicing_intronic variant will only span -------[===]----, with * representing the variant). Similarly, the windows of intronic variants will span two introns and a central exon (the one before the variant, as in the splicing_intronic "sketch" above).

I believe what we had discussed was to keep exonic, splicing_exonic, and splicing_intronic variants so that their windows would span from the start of the left flanking exon to the end of the right flanking exon (as shown in the splicing_exonic "sketch") while changing the window for intronic variants to only spanning the intron in which the variant resides (or rather, the superset of such introns across transcripts for which the variant is "intronic"). Or am I misremembering? If not, I've proposed some changes in the code to address these concerns.

@malachig, @gatoravi

Show outdated Hide outdated src/variants/variants_annotator.cc Outdated
Show outdated Hide outdated src/variants/variants_annotator.cc Outdated
Show outdated Hide outdated src/variants/variants_annotator.cc Outdated
Show outdated Hide outdated src/variants/variants_annotator.cc Outdated
Show outdated Hide outdated src/variants/variants_annotator.cc Outdated
Show outdated Hide outdated src/variants/variants_annotator.cc Outdated
Show outdated Hide outdated src/variants/variants_annotator.cc Outdated
Show outdated Hide outdated src/variants/variants_annotator.cc Outdated
@gatoravi

This comment has been minimized.

Show comment
Hide comment
@gatoravi

gatoravi Sep 11, 2017

Contributor

Thanks for looking this over @yanyangfeng ! How does this look, I only do this for the 'intronic' option now, the variant region is just the immediate intronic space where the variant lies. The other options are same as before, with the boundaries including the flanking exons. I also modified the code to allow partial overlap of junctions with the regions, so if either the junction start or junction end lies within the region it get's reported. Previously a junction had to be contained entirely within the window to be reported.

Contributor

gatoravi commented Sep 11, 2017

Thanks for looking this over @yanyangfeng ! How does this look, I only do this for the 'intronic' option now, the variant region is just the immediate intronic space where the variant lies. The other options are same as before, with the boundaries including the flanking exons. I also modified the code to allow partial overlap of junctions with the regions, so if either the junction start or junction end lies within the region it get's reported. Previously a junction had to be contained entirely within the window to be reported.

@yang-yangfeng

This comment has been minimized.

Show comment
Hide comment
@yang-yangfeng

yang-yangfeng Sep 18, 2017

Contributor

@gatoravi seems like there were a few small bugs causing errors in dealing with "intronic" variants. I pushed a commit to address these - take a look and see if you agree with my changes? As for the window overlap change - I'm wondering if we should also allow partial overlaps when the user provides a window size? I feel like it might be confusing to the user otherwise.

Contributor

yang-yangfeng commented Sep 18, 2017

@gatoravi seems like there were a few small bugs causing errors in dealing with "intronic" variants. I pushed a commit to address these - take a look and see if you agree with my changes? As for the window overlap change - I'm wondering if we should also allow partial overlaps when the user provides a window size? I feel like it might be confusing to the user otherwise.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 19, 2017

Coverage Status

Coverage decreased (-0.1%) to 89.197% when pulling 67ce56c on gatoravi:fix_E_v2 into d77dad5 on griffithlab:master.

coveralls commented Sep 19, 2017

Coverage Status

Coverage decreased (-0.1%) to 89.197% when pulling 67ce56c on gatoravi:fix_E_v2 into d77dad5 on griffithlab:master.

@gatoravi

This comment has been minimized.

Show comment
Hide comment
@gatoravi

gatoravi Oct 3, 2017

Contributor

Hi @yanyangfeng, sorry for the slow response on this, this looks good to me. If you think partial overlaps might make it easier for the user I agree we should go for it.

Contributor

gatoravi commented Oct 3, 2017

Hi @yanyangfeng, sorry for the slow response on this, this looks good to me. If you think partial overlaps might make it easier for the user I agree we should go for it.

@yang-yangfeng

This comment has been minimized.

Show comment
Hide comment
@yang-yangfeng

yang-yangfeng Oct 9, 2017

Contributor

Just want to document this here: seems the reason the coverage dropped was because some conditionals were never true so their blocks were never being executed. Somehow, though, the CI builds still passed, so it seems like there are some cases that our leaking through our tests. We may want to revisit our tests at some point to make them more robust.

Contributor

yang-yangfeng commented Oct 9, 2017

Just want to document this here: seems the reason the coverage dropped was because some conditionals were never true so their blocks were never being executed. Somehow, though, the CI builds still passed, so it seems like there are some cases that our leaking through our tests. We may want to revisit our tests at some point to make them more robust.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 9, 2017

Coverage Status

Coverage increased (+0.4%) to 89.687% when pulling dfb6b1b on gatoravi:fix_E_v2 into d77dad5 on griffithlab:master.

coveralls commented Oct 9, 2017

Coverage Status

Coverage increased (+0.4%) to 89.687% when pulling dfb6b1b on gatoravi:fix_E_v2 into d77dad5 on griffithlab:master.

@yang-yangfeng yang-yangfeng merged commit 9da076e into griffithlab:master Oct 9, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.4%) to 89.687%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment