fix code duplication in ReadPosRankSumTest and its allele-specific version #2657
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2657 +/- ##
==============================================
- Coverage 80.453% 80.45% -0.003%
+ Complexity 17522 17521 -1
==============================================
Files 1173 1173
Lines 63415 63406 -9
Branches 9879 9875 -4
==============================================
- Hits 51019 51010 -9
Misses 8447 8447
Partials 3949 3949
|
|
I'll do the review. This is a great opportunity for me to get my feet wet
in Gatk4.
|
| public class AS_ReadPosRankSumTest extends AS_RankSumTest implements AS_StandardAnnotation { | ||
|
|
||
| final ReadPosRankSumTest readPosRankSumTest = new ReadPosRankSumTest(); |
There was a problem hiding this comment.
I really wanted this class to inherit from AS_RankSumTest and ReadPosRankSumTest, so I picked one and copied the code from the other. This is far more elegant.
There was a problem hiding this comment.
I feel like the real way to do this is via a decorator that makes some annotation allele-specific, but I have no idea how to do that in Java.
There was a problem hiding this comment.
It looks to me like getElementForRead(), at least, could be made static (or rather, you could have getElementForRead() delegate to a static method). I suspect that isUsableRead() could be refactored to call into a static method as well. Then you wouldn't have to instantiate ReadPosRankSumTest like this here.
There was a problem hiding this comment.
@droazen I couldn't figure out a way to beat code duplication for isUsableRead -- since you can't override static methods, every option was a lot more fuss than it was worth. I made getElementForRead delegate to a static method in order to avoid instantiating a ReadPosRankSumTest.
|
Ha, this PR was much tinier than I expected -- feet are barely damp. My "wishlist" would include a much bigger refactor because I made a mess in Java7 and didn't have the time to clean up (especially with generics) after we switched in Java8. I'm still tinkering with the rank sum tests though, so it's not worth tackling the refactor until those are good to go. 👍 |
|
@droazen am I waiting for your approval? |
|
@droazen waiting for your thumbs to be directed toward the stars. |
davidbenjamin
force-pushed
the
db_issue_1882
branch
from
288b7c0
to
f1acf1f
Compare
|
@droazen Is it okay to merge this PR? |
|
@davidbenjamin I'll have a look at this one later today. |
davidbenjamin
force-pushed
the
db_issue_1882
branch
3 times, most recently
from
787d2a8
to
8ace546
Compare
davidbenjamin
force-pushed
the
db_issue_1882
branch
from
8ace546
to
314a8d5
Compare
davidbenjamin
force-pushed
the
db_issue_1882
branch
from
314a8d5
to
0ab877a
Compare
|
Rebased -- should still pass tests. |
|
I think this is good to go, @droazen . |
|
@lbergelson do you think you could approve this while David is away? |
|
@davidbenjamin Sorry to get in your way, I see that this has been sitting forever. I know that @jamesemery is working on this same code right now. I'm going to pass this one to him so he can have a heads up about this change. |
|
@jamesemery Could you take a look at this change. |
Closes #1882.
Normally I assign the author of the issue for this kind of stuff, but Adam is no longer with us. @ldgauthier would you mind looking and/or suggesting anything else on your wish list for the annotations code?