More fix enum hashCode #4623
More fix enum hashCode #4623
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4623 +/- ##
===============================================
- Coverage 79.821% 79.813% -0.008%
Complexity 17152 17152
===============================================
Files 1067 1067
Lines 62405 62415 +10
Branches 10126 10130 +4
===============================================
+ Hits 49812 49815 +3
- Misses 8648 8655 +7
Partials 3945 3945
|
| @@ -10,4 +34,213 @@ | |||
| public abstract class AlignedContigGenerator { | |||
There was a problem hiding this comment.
It seems like this class should really just be an interface? The two subclasses could be top-level classes that just implement the interface. It's a bit weird to have this be an abstract class when it really only defines the signature of one method.
There was a problem hiding this comment.
Agree. But in 2nd thought, this is not related to this fix, so I'll do the refactor and the move in another PR.
I'm trying to put the implementations in one place and see if I can have one concrete class, just to avoid the two code path that exist mostly for historical reasons.
| @@ -207,11 +204,15 @@ private SvDiscoveryInputData getSvDiscoveryInputData(final JavaSparkContext ctx, | |||
| final String outputPrefixWithSampleName = variantsOutDir + (variantsOutDir.endsWith("/") ? "" : "/") | |||
| + SVUtils.getSampleId(headerForReads) + "_"; | |||
|
|
|||
| final JavaRDD<GATKRead> reloadedContigAlignments = new ReadsSparkSource(ctx, readArguments.getReadValidationStringency()) | |||
There was a problem hiding this comment.
I don't see how moving the two classes to live in the abstract class would change anything, so maybe this is what actually resolved the behavior for you? Before, how was getReads not returning the raw reads in the original bam file?
There was a problem hiding this comment.
That getReads() call was not right, and luckily it was actually not used so I'm going to fix it in a followup cleanup and test coverage bump PR.
| for (final Strand strand : dupSeqStrandOnCtg) { | ||
| result = 31 * result + Objects.hashCode( strand.ordinal() ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the actual fix, I believe. Chris and I missed the list of enums. (A List's hashCode combines the hashCode's of its elements, and that's no good on Spark.)
SHuang-Broad
force-pushed
the
sh_more_fix_enum_hascodes
branch
from
03b460f
to
e947f5e
Compare
| result = 31 * result + Objects.hashCode( strand.ordinal() ); | ||
| } | ||
| for (final Strand strand : dupSeqStrandOnCtg) { | ||
| result = 31 * result + Objects.hashCode( strand.ordinal() ); |
There was a problem hiding this comment.
Objects.hashCode(strand.ordinal()) is a very expensive way of calculating strand.ordinal(). Maybe just use strand.ordinal() here, and on lines 1081 and 1086 below.
There was a problem hiding this comment.
Oh, you're right, that was dumb of me. @SHuang-Broad, can you change that to just ordinal() instead of Objects.hashCode(strandSwitch.ordinal()) in the other places I put it in as well (AlignmentInterval, CpxVariantInducingAssemblyContig, ChimericAlignment, and NovelAdjacencyAndAltHaplotype)? Thanks.
There was a problem hiding this comment.
I should have noticed this on Chris' earlier check-in, too, but didn't. It also has this unnecessary indirection through Objects.hashCode. Sorry.
There was a problem hiding this comment.
Thank you both! It's been done now.
SHuang-Broad
force-pushed
the
sh_more_fix_enum_hascodes
branch
from
e947f5e
to
bf0b479
Compare
SHuang-Broad
force-pushed
the
sh_more_fix_enum_hascodes
branch
from
bf0b479
to
344db59
Compare
|
Verified to be running correctly and not generating duplicates anymore. |
More fix on hashcode functions of structs that contain enums
in
BreakpointComplicationsAfter this fix, the duplicated records disappear.