refactoring some stuff for clarity #8598

lbergelson · 2023-12-04T17:00:34Z

I removed an unused type parameter and refactored some functionality into a class MultiPloidyGenotyper which handles dealing with multiple ploidies in a somewhat unified way. It could probably be renamed and expanded slightly.

gatk-bot · 2023-12-04T17:56:04Z

Github actions tests reported job failures from actions build 7089943546
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
variantcalling	17.0.6+10	7089943546.2	logs

rickymagner

This looks great to me overall. Thanks for refactoring this stuff! This will definitely help keep some of the ploidy-specific logic encapsulated away from the rest of the HC code. I just had a few quick questions in my review to clarify.

I think it looks good to me to merge. Not sure if e.g. @jamesemery wants to take a look, since some other HC code was tweaked a little here as well.

.../java/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/HaplotypeCallerEngine.java

rickymagner · 2023-12-04T18:58:14Z

...n/java/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/MultiPloidyGenotyper.java

+   }
+
+    public T getDefaultGenotypingEngine(){
+        return ploidyToGenotyperMap.get(DEFAULT_PLOIDY_SENTINEL);


Is there a good reason to keep this method separate from the other generic getGenotypingEngine, since the getPloidyToUseAtThisSite covers the default sentinel value case?

I included it because there was something that needed the "default" genotyper btt didn't necessarily know what the default ploidy was in that location I think. I would prefer to remove the `DEFAULT_PLOIDY_SENTINEL" entirely and just have it be based on the ploidy number, maybe save the default ploidy somewhere but I didn't do it yet.

jamesemery

Pulling the logic to its own class seems reasonable. Otherwise I question the value of many of these refactors as they don't seem to provide much clarity. A few specific refactors that are not worth making here.

jamesemery · 2023-12-05T18:37:57Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypingEngine.java

-     */
-    private static class OutputAlleleSubset {
-        private  final List<Allele> alleles;
-        private  final boolean siteIsMonomorphic;


i know this is wrong and some ancient detritus... don't open this can of worms here... its not worth the trouble and we have to do this quick

This is a completely automated refactor so it just saves a few lines of code and does the same thing, but I can revert it.

After discussion I've kept this change.

jamesemery · 2023-12-05T18:40:01Z

...org/broadinstitute/hellbender/tools/walkers/haplotypecaller/RampedHaplotypeCallerEngine.java

@@ -145,7 +145,7 @@ private void tearRamps() {
            throw new RuntimeException(e);
        }
    }
-    private class CallRegionContext {


Because it had a squiggle in IntelliJ? Always static all the time.

jamesemery · 2023-12-05T18:41:24Z

...n/java/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/MultiPloidyGenotyper.java

+import java.util.Set;
+import java.util.function.IntFunction;
+
+public class MultiPloidyGenotyper<T extends GenotypingEngine> {


also consider naming this to be a little more clear. This is not a genotyper this is a ploidy switch for genotypers

Yeah, I was hoping someone else would name it :)

jamesemery · 2023-12-05T18:48:06Z

.../java/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/HaplotypeCallerEngine.java

+
+        activeRegionGenotypingEngines = new MultiPloidyGenotyper<>(ploidy -> {
+                // Seems that at least with some test data we can lose genuine haploid variation if we use ploidy == 1
+                final int adjustedPloidy = Math.max(MINIMUM_PUTATIVE_PLOIDY_FOR_ACTIVE_REGION_DISCOVERY, ploidy);


maybe consider pulling this lambda out to be named and defined somewhere.

…ial bug, see the todo in the tests

lbergelson · 2023-12-06T21:57:13Z

.../java/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/HaplotypeCallerEngine.java

@@ -357,6 +334,13 @@ private void initialize(boolean createBamOutIndex, final boolean createBamOutMD5
                : null);
    }

+    private HaplotypeCallerGenotypingEngine makeGenotypingEngine(int ploidy) {


I pulled this out like you I was asked, I didn't pull out the one for the minimal genotyper because it relied on state from the function around it and it seemed messier to do so. I'm hoping these can be simplified when you put in the copy methods.

lbergelson · 2023-12-06T21:58:31Z

...a/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/MultiPloidyGenotyperCache.java

+ * It is lazily initialized so GenotypingEngines are only created on as needed.
+ * @param <T> the GenotypingEngine implementation this provides
+ */
+public final class MultiPloidyGenotyperCache<T extends GenotypingEngine> {


@jamesemery Could you re-review this? I rewrote it to be less shitty and added documentation / renamed it.

lbergelson · 2023-12-06T22:02:42Z

...oadinstitute/hellbender/tools/walkers/haplotypecaller/MultiPloidyGenotyperCacheUnitTest.java

+                {"1:10-30", 2},
+                {"1:70-85", 3},
+                {"1:80-93", 3},
+              //  {"1:89-92", 2},  TODO is this a bug?


So I think we have a bug. Maybe it's not in the original code but I'm thinking it probably is. If you have a region which contains both default ploidy 2 and a special ploidy 1 region then you end up calling that at ploidy 1 because it's picking the highest of the ALTERNATE ploidies, not the highest of any overlapping ploidy.

@jamesemery @rickymagner

Hmm you mean the bed file input has a region of ploidy 1 and an overlapping region explicitly listed as ploidy 2 (which happens to be the global default)? And then in that case the ploidy actually becomes 1 in practice?

I'm not sure what the "right" thing I'd want to be here, and not sure it matters so much anyway (since I think we agreed that what happens on the boundary is bound to be a little wacky and semi-unpredictable anyway). I'd be OK rewording the documentation to say "highest of the non-default alternate ploidies," or to try reworking the logic so in this case the ploidy 2 explicitly provided takes precedence.

When we poll the regions we are generally polling them on a per-variant basis no? That was my understanding from the baranch. If I have a deletion that overlaps a region that I called ployidy 1 and also overlaps something i haven't changed at all I would very much think the best behavior in that case is to call it as ployidy 1? Am i missing something? This does not seem like a bug to me. We are doing the most specific thing the user has specified in this case.

I think the case Louis was referring to was a hypothetical user-provided bed file that looked like:

chr1 1000 1700 1 chr1 1500 2000 2

Then maybe the current behavior would be to still use ploidy 1 in the 1500-1700 region even though the "2" overlaps there, because the 2 is the built-in default? Is this correct Louis?

private int getPloidyToUseAtThisSite(Locatable region) { return ploidyRegions.getOverlaps(region) .stream() .mapToInt(SimpleCount::getCount) .max() .orElse(defaultPloidy);

Am I wrong in interpreting that in that case this would fall into the max() command? There is a simple-count object for both chr1 1500 2000 2 and chr1 1000 1700 1 and it would fall into 2. The only time default ever matters is if there is nothing specified that covers the site?

Ricky and I chatted and it seems like it's working as intended. Used the explicitly specified ploidy if a region overlaps one even if it includes default region which is higher.

gatk-bot · 2023-12-06T22:11:12Z

Github actions tests reported job failures from actions build 7120721554
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
cloud	17.0.6+10	7120721554.10	logs
variantcalling	17.0.6+10	7120721554.2	logs

gatk-bot · 2023-12-06T22:16:20Z

Github actions tests reported job failures from actions build 7120763686
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
cloud	17.0.6+10	7120763686.10	logs
integration	17.0.6+10	7120763686.11	logs
variantcalling	17.0.6+10	7120763686.2	logs
integration	17.0.6+10	7120763686.0	logs

jamesemery

The cache class looks better now. This looks good to me

* Introduced a new class MulitPloidyGenotyperCache which keeps track of multiple GenotypingEngines that have different ploidy values. * minor refactoring in related classes * removed an unused type parameter from GenotypingEngine

This reverts commit a40ba3e.

refactoring some stuff for clarity

3735caa

lbergelson requested a review from jamesemery December 4, 2023 17:00

rickymagner reviewed Dec 4, 2023

View reviewed changes

jamesemery reviewed Dec 5, 2023

View reviewed changes

lbergelson added 2 commits December 6, 2023 16:22

further improvements

63593fc

Adding tests and further refactors, note that I've disovered a potent…

1df4aa2

…ial bug, see the todo in the tests

lbergelson commented Dec 6, 2023

View reviewed changes

unfix minor thing in RampedHaplotypeCallerEngine

fdca109

lbergelson commented Dec 6, 2023

View reviewed changes

jamesemery approved these changes Dec 7, 2023

View reviewed changes

lbergelson merged commit 4cf1052 into tkay_PARploidy Dec 7, 2023
15 of 20 checks passed

lbergelson deleted the lb_refactor_ricky_stuff branch December 7, 2023 16:38

lbergelson added a commit that referenced this pull request Dec 8, 2023

Revert "Extracted a cache of Genotypers indexed by ploidy (#8598)"

a99eae2

This reverts commit a40ba3e.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactoring some stuff for clarity #8598

refactoring some stuff for clarity #8598

lbergelson commented Dec 4, 2023

gatk-bot commented Dec 4, 2023

rickymagner left a comment

rickymagner Dec 4, 2023

lbergelson Dec 5, 2023

jamesemery left a comment

jamesemery Dec 5, 2023

lbergelson Dec 5, 2023

lbergelson Dec 6, 2023

jamesemery Dec 5, 2023

lbergelson Dec 5, 2023

jamesemery Dec 5, 2023

jamesemery Dec 5, 2023

lbergelson Dec 5, 2023

jamesemery Dec 5, 2023

lbergelson Dec 6, 2023

lbergelson Dec 6, 2023

lbergelson Dec 6, 2023

lbergelson Dec 6, 2023

rickymagner Dec 7, 2023

jamesemery Dec 7, 2023

rickymagner Dec 7, 2023

jamesemery Dec 7, 2023

lbergelson Dec 7, 2023

gatk-bot commented Dec 6, 2023 •

edited

Loading

gatk-bot commented Dec 6, 2023 •

edited

Loading

jamesemery left a comment

refactoring some stuff for clarity #8598

refactoring some stuff for clarity #8598

Conversation

lbergelson commented Dec 4, 2023

gatk-bot commented Dec 4, 2023

rickymagner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesemery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatk-bot commented Dec 6, 2023 • edited Loading

gatk-bot commented Dec 6, 2023 • edited Loading

jamesemery left a comment

Choose a reason for hiding this comment

gatk-bot commented Dec 6, 2023 •

edited

Loading

gatk-bot commented Dec 6, 2023 •

edited

Loading