JAL-2742: Out by one error in CigarArray constructor

Activity

CR-JAL-105 17

Keyboard shortcuts  
  • Summarize the review outcomes (optional)
     
    #permalink

    Details

    Warning: no files are visible, they have all been filtered.
    Participant Role Time Spent Comments Latest Comment
    Author 21m 3 I've removed getViewAsCigars. Remainder seems a rather la...
    Reviewer - Complete 23m 12 You reach it from AlignmentView constructor when there ar...
    Total   44m 17  
    #permalink

    Objectives

    The CigarArray constructor builds a CigarArray based on a selection group and a set of hidden columns: hidden columns within the bounds of the selection group are marked as D.

    However if the selection group starts at the same column as a hidden columns region ends i.e. overlap of 1 column, the CIGAR string is created as if the column were not hidden. E.g.
    selection group = [6,23]
    hidden columns = [3,6],[16,20]
    resulting CIGAR string: 10M5D3M (should be 1D9M5D3M)

    selection groups starting earlier give expected results of:
    selection group = [5,23] : CIGAR = 2D9M5D3M
    selection group = [4,23] : CIGAR = 3D9M5D3M
    selection group = [3,23] : CIGAR = 4D9M5D3M

    Branches in review

    #permalink

    Issues Raised From Comments

    Key Summary State Assignee
    #permalink

    General Comments

    Mungo Carstairs

    Looking for possible impact of the bug: *the CigarArray constructor is call...

    Looking for possible impact of the bug:

    • the CigarArray constructor is called from AlignmentView constructor, and also from AlignmentViewport.getViewAsCigars
    • is it possible to add a failing unit test for the former that is fixed by the change?
    • getViewAsCigars is not used anywhere - should it be removed?

    Kira Mourão

    I've removed getViewAsCigars. Remainder seems a rather larger piece of work -...

    I've removed getViewAsCigars. Remainder seems a rather larger piece of work - will add as a new issue along with CigarArray documentation and unit testing.

    Mungo Carstairs

    I always find Cigar code mysterious - so will sign off unit tests as ok and p...

    I always find Cigar code mysterious - so will sign off unit tests as ok and passing, subject to comments as to possible additional test coverage.

    Some decent class Javadoc for Cigar, CigarBase, SeqCigar etc would be very helpful.

    /src/jalview/datamodel/CigarArray.java Changed 10
    /src/jalview/datamodel/CigarArray.java Changed
    Open in IDE #permalink
    /test/.../datamodel/CigarArrayTest.java Added 4

    Review updated: Reload | Ignore | Collapse

    You cannot reload the review while writing a comment.

    Log time against