search

usadellab / Trimmomatic

Other
217 stars 70 forks source link

Inconsistency between design and implementation caused by misconception about ListIterator's previous() method #28

Open zhangzhen opened 2 years ago

zhangzhen commented 2 years ago

Inconsistency between design and implementation is found in the code for the calculateMaximumRange() method in the IlluminaClippingSeq abstract class. https://github.com/usadellab/Trimmomatic/blob/156c1a871a1cb5ca8e143b7d35ee7c6ba630e7e1/src/org/usadellab/trimmomatic/trim/IlluminaClippingTrimmer.java#L589-L608 prev gets the same value as val in line 591 because ListIterator's previous method returns the current value and then moves backwards.

val < 0 in line 589. prev > -val in line 595 means val >0 if prev were equal to val. Since val < 0 and val >0 cannot coexist, the if block in lines of 596-604 will never be executed. Hence merges will never be changed. I think this is not what the author wants to do. Is this really a bug?

Zhen

TonyBolger commented 2 years ago

Good find - this code as implemented doesn't actually perform local score merges. Then again, even if it did, local score merging alone doesn't ensure an optimal global result.

zhangzhen commented 2 years ago

local score merging alone doesn't ensure an optimal global result

How will you address this problem?

Regards, Zhen