Fix get_end VCF parser - ENSINT-2005

[sgiorgetti]

Changes so far

• patched get_end (not for SV)
• fixed test suite and re-arranged test data

NOTES - This is still draft

• These changes are on top of release/114
• @nakib103 to confirm it is OK, as now get_end and get_raw_end are the same unless $info->{SVLEN}
• add tests to check get_end Vs get_raw_end esp. when the two differ ... when?

[nakib103]

@sgiorgetti,

About the question that get_end and get_raw_end becoming the same functionality, I think that makes sense. In this wiki we can see that in VCF with correct normalization, the alleles can never end in same nucleotide. Hence we should not need to do trimming on the right (and the parser do not do any such thing). As there is no operation done on on the right, it should be correct if both of the function return the same value.

PS: with the latest change request I made the get_end checks an extra case. The get_raw_end function should also get that case (although it is not affecting us too much as I saw get_raw_end used for non-SV cases in our code, but it would be more correct to do so).

or, better, if we add all the changes to get_end, from the arguments made above - we can call call get_end from get_raw_end function and return the same result.

[sgiorgetti]

@nakib103 - I have updated the code, basically as per your suggestions (thanks!). Would you mind to double check the logic of the changes?

@TamaraNaboulsi - would you mind to have a look at these changes, should I have missed anything?

Thank you both!

[nakib103]

@sgiorgetti,

Regarding - https://github.com/Ensembl/ensembl-io/pull/171/files#r1814776276

1. The expected change would be to change the order in the if-elsif like so,

currently we have -

    if (defined($info->{END})) {
      $end = $info->{END};
    }
    elsif(defined($info->{SVLEN})) {
      return unless $self->get_start;
      my $svlen = (split(',',$info->{SVLEN}))[0];
      $end = $self->get_start + abs($svlen) - 1;
    }

change it to -

    if (defined($info->{SVLEN})) {
      return unless $self->get_start;
      my $svlen = (split(',',$info->{SVLEN}))[0];
      $end = $self->get_start + abs($svlen) - 1;
    }
    elsif (defined($info->{END})) {
      $end = $info->{END};
    }

So we calculate end using SVLEN first if available.

2. Also, for sub get_raw_end, it is better to change it to call get_end as all these changes should be added in get_raw_end too (but we do not want to duplicate effort) -

sub get_raw_end {
    my $self = shift;
    return $self->get_end;
}

[sgiorgetti]

Yes ... and you mentioned that before ... bear with me

[nakib103]

thanks @sgiorgetti !

[nakib103]

Hi @sgiorgetti , thanks a lot for the changes! There still one change request that needs to be addressed. I have added that above.

Also tagging @likhitha-surapaneni as she will also be reviewing the changes.

[sgiorgetti]

Re-requested review to confirm - also added @likhitha-surapaneni explicitly :-)

[TamaraNaboulsi]

Change includes:

• Updated the get_raw_end method to be equivalent to get_end
• Updated get_end method to include alternative sequences, checks for additional info (SVTYPE), and changing the order of checking SVLEN vs END
• Updated the test file with some clarity in assigning the test_sample and added a new test for loading the 7th record

Looks good to me and all requests were resolved.

[likhitha-surapaneni]

Thank you for the fix @sgiorgetti, the code works as expected for the examples tested. Currently, testing on a larger SV VCF file to check that there are no breaking changes.

[likhitha-surapaneni]

Tested with larger file and looks good to me

[sgiorgetti]

Thanks @likhitha-surapaneni ! Shall go ahead and merge this in.