search

genome-rcast / karkinos

Tumor genotyper, that detects SNV, absolute CNV and Tumor contents
Apache License 2.0
10 stars 2 forks source link

Unify different ranges into 1-based closed range #5

Closed r6eve closed 5 years ago

r6eve commented 5 years ago

Karkinos treats different ranges something like BED format (0-based half-opened range) and SAMRecord of htsjdk (1-based closed range), but doesn't convert those into specified range. This PR enables karkinos to unify those intervals into 1-based closed range.

This changes includes the followings.

r6eve commented 5 years ago

Thank you for pointing out my mistake. I've fixed it accordingly.

alumi commented 5 years ago

@r6eve Thank you for the fixes! Please give me some more time to check if there're no degradations.

r6eve commented 5 years ago

Sorry, I forgot to unify some points in refGene.txt. I'll make test and fix it. https://github.com/genome-rcast/karkinos/blob/f6bca10f0821d4042d46c0f1cd971882d60f5380/src/main/java/jp/ac/utokyo/rcast/karkinos/readssummary/GeneExons.java#L114-L149

r6eve commented 5 years ago

I'm super sorry to confuse you, it's my over-thinking. I've added testcases for refGene.txt.

r6eve commented 5 years ago

I've refactored method which loads refGene.txt and added comments for future work. Please review it again.

r6eve commented 5 years ago

Thank you for your detailed comments. I've fixed it.

r6eve commented 5 years ago

Sorry, again. I forgot to check subMap. I've fixed it and added the tests.

r6eve commented 5 years ago

I've unified the interval range and fixed the bug that doesn't extend the Interval#end correctly.

r6eve commented 5 years ago

I've introduced the CopyNumberIntarva#length method which calculates the length correctly.

r6eve commented 5 years ago

In previous implementation (i.e. current master branch), there is a bug that counter.get(Object key) always returns a constant value. This is because the counter always puts same key "" which is returned by symbolWithoutNum(data[0]).

I confirmed this bug has already been fixed by 158226192ca8a36474b299e2837de8aac399dcb3 . And, I added unit tests to check it.

r6eve commented 5 years ago

I think all points have been clarified. Please review it again.

r6eve commented 5 years ago

Thank you for pointing out. I fixed it.

alumi commented 5 years ago

Merged. Thank you for the great improvement!

r6eve commented 5 years ago

I appreciate all your reviews. Thank you.