Assumption made in code masked by earlier check.

[statgen]

Assumption made in code masked by earlier check.

[pjvandehaar]

Thanks for reporting this. Unfortunately, I don't understand yet. Could you explain more? SamRecord.get0BasedPosition() returns the leftmost position of the record, the first record always starts before the second, and CigarHelper::softClipEndByRefPos handles forward/reverse reads correctly, right?

[innovate-invent]

the first record always starts before the second

is the assumption I am referring to. This could fail given an exotic data set.

firstRecord                    ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
secondRecord  |||||||||||||||||||||||||||||||||||||||||||||||||

is just one possibility if the data is not sorted by coordinate.

[pjvandehaar]

Thanks for clarifying that. Would an assertion be a good approach?

if (firstRecord.get0BasedPosition() > secondRecord.get0BasedPosition()) {
 std::ostringstream errmsg;
 errmsg 
  << "BamUtil's OverlapClipLowerBaseQual::handleOverlapPair encountered paired reads"
  << " in the wrong order where firstRecord's leftmost 0-based position is "
  << firstRecord.get0BasedPosition()
  << " and secondRecord's leftmost 0-based position is "
  << secondRecord.get0BasedPosition()
  << std::endl;
 throw(std::runtime_error(errmsg.str()));
}

Or just fixing the problem?

void OverlapClipLowerBaseQual::handleOverlapPair(SamRecord& firstRecord,
                                                 SamRecord& secondRecord)
{
  SamRecord *first = &firstRecord;
  SamRecord *second = &secondRecord;
  if (first->get0BasedPosition() > second->get0BasedPosition()) {
    SamRecord *tmp = first;
    first = second;
    second = tmp;
  }
  // replace `firstRecord.` with `first->` and `f(firstRecord)` with `f(*first)`
}

[innovate-invent]

Depending on how robust you want the code to be you have a few options.
Ideally you would handle any data set by just comparing the start and ends of each record and retain the inner region.
Something along the lines of

    int32_t overlapStart = firstRecord.get0BasedPosition() > secondRecord.get0BasedPosition() ? firstRecord.get0BasedPosition() : secondRecord.get0BasedPosition();
    int32_t overlapEnd = firstRecord.get0BasedAlignmentEnd() > secondRecord.get0BasedAlignmentEnd() ? secondRecord.get0BasedAlignmentEnd() : firstRecord.get0BasedAlignmentEnd();

I haven't fully dissected the rest of this function to understand how this change would affect it otherwise I would just submit a pull request.

Given my assumption of how the function handles these variables it should be able to handle my previous example as well as

first               ||||||||||||||||
second  |||||||||||||||||||||||||||||||||||||

or

first       |||||||||||||||||||||||||||||||||||||
second             ||||||||||||||

[pjvandehaar]

It already handles

first       |||||||||||||||||||||||||||||||||||||
second             ||||||||||||||

here

[innovate-invent]

Unless the average quality of second is greater.