Sam record pair identification depends on required SAM fields #994
Conversation
| import org.testng.Assert; | ||
| import org.testng.annotations.Test; | ||
|
|
||
| public class SAMRecordTest { |
There was a problem hiding this comment.
Unfortunately it looks like SAMRecordUnitTest is the class where tests for SAMRecord are stored. If so, you should either move those tests here or these tests there.
Codecov Report
@@ Coverage Diff @@
## master #994 +/- ##
===============================================
+ Coverage 66.039% 66.063% +0.024%
- Complexity 7477 7485 +8
===============================================
Files 529 529
Lines 31963 31971 +8
Branches 5457 5464 +7
===============================================
+ Hits 21108 21121 +13
+ Misses 8713 8706 -7
- Partials 2142 2144 +2
|
|
I am a bit time constrained at the moment. I strongly object to this PR on my first read. Can you wait until early next week for me or a maintainer to take a look? |
| @@ -334,6 +334,19 @@ private boolean hasReferenceName() { | |||
| } | |||
|
|
|||
| /** | |||
| * @return {@code true} if records belong to the same pairwise alignment | |||
| */ | |||
| public boolean isPair(final SAMRecord record) { | |||
There was a problem hiding this comment.
I'm confused about this function, a mate is another record with the same query name...why are you checking everything else, and not the QNAME? I'm sure there's a reason, but it should be explained in the javadoc as it is not obvious at all!
There was a problem hiding this comment.
Thank you for the remark, I think it does matter in SamRecord and I fixed it, but if we implement it in HitsForInsert it doesn't matter because it deals records with the same QNAME only. Here is the logic that produces HitsForInsert.
There was a problem hiding this comment.
I would suggest to move this to SamPairUtil, because there are already a lot of methods in SAMRecord that may be better to be used within an utility class. Eventually, reads would be accessed by an interface (e.g., #985) and thus this methods may live in an utility class that is implementation-independent.
|
could you explain in the description what problem this is solving? what is the purpose of this new function? |
|
yes, but a public function with the name |
There was a problem hiding this comment.
I don't this PR is likely to be merged ever. It seems to me that this method is intended for a very specific purpose and is unsuitable for inclusion in SAMRecord. It appears to be asking "is this other record related to, and meeting some arbitrary definition of properly paired with, this record".
DenisVerkhoturov
force-pushed
the
epam-ls_SamRecordPairIdentification
branch
from
20ad2c9 to
3c01a88
Compare
There was a problem hiding this comment.
I also don't really understand this method and it's purpose. Two reads with the same name are by definition a pair, unless they're supplementary or secondary reads. I assume this is intended to be use for some sort of validation? It should probably take into account supplementary / secondary reads if it's going to be part of the public api. It also doesn't account for many segment reads, which are admittedly poorly supported in htsjdk, but we should try to make an effort to deal with in new code.
I agree with the other commenters that this doesn't belong in SAMRecord, it could maybe go into one of the utils classes if it's named more clearly and the javadoc explains what's for better.
I hate to reject your effort, especially since you did the work of including tests (thank you for that!), but it seems like it's probably something for a very specific use case though and maybe it's best to just leave it as private method wherever it's used.
|
Thank everybody for comments!=) |
|
@DenisVerkhoturov I think that's the right way to proceed. Thank you for your PR and interest in the project! We welcome future contributions. |
Description
Associated pull-request, please, follow the link for details.
Checklist