handle wes/wgs inheritance edge case

[jklugherz]

No description provided.

[jklugherz]

these binds are a little wild, but the logic is that for all families, keep family if all of its samples pass this check:
if sample failed in one sample type, it must pass in the other - and if a sample for that family failed in the other sample type, it must pass in the current sample type

[hanars]

Looking at this, I think we may also need extra logic for comp hets, but I would do that as a separate follow up PR after this is done to aooid scope creep

[hanars]

This feels incredibly similar to _annotate_families_inheritance. Rather than making a whole separate function for this, you could make a much more tightly scoped conditional helper to pass into _annotate_families_inheritance, perhaps just for the lambda function applied to the hl.enumerate(ht[entries_ht_field]).starmap(

[jklugherz]

I implemented this, but because I don't want the single sample type families that come originally through the mito class code path to call the mito 'family_passes_inheritance_filter' function, I'm still passing a family_passes_inheritance_filter function to _filter_inheritance instead of using python inheritance here. Do you know if there's a cleaner way to do this?

[hanars]

So this is testing the case where its not returned when theres no valid WGS parental data, but can we also test that it IS returned when we include the parental data for WGS and its overridden?

[jklugherz]

I think this case is covered by an above test -

    # Variant1 in family_2 is de novo in exome but maternally inherited in genome.
    # Genome passes quality and inheritance, show genotypes for both sample types.

[hanars]

The search criteria changes between these 2 test cases. We really need back-to-back tests that the exact same search returns different results when the paternal data is present/absent. Instead of running a dominant and recessive search each filtered to variant 1 and variant 2, it would probably be better to not use an interval filter at all and test that dominant and recessive search return the expected combo of variants with and without paternal data

[jklugherz]

I did this^!

[hanars]

string comparisons in hail are substantially slower than integer comparisons. family_entries_field and failed_family_sample_field for a given sample type should be the same length and the same order so you should be able to rewrite this more performantly as

hl.enumerate(ht[sample_type.other_sample_type.family_entries_field][other_sample_type_family_idx]).starmap(
  lambda i, s: hl.or_missing(
    hl.is_missing(ht[sample_type.other_sample_type.failed_family_sample_field][other_sample_type_family_idx][i]),
    s['sampleId'],
  )
)

[jklugherz]

I think this won't work since family_entries_field and failed_family_sample_field for a given sample type are not necessarily the same length. ht[sample_type.other_sample_type.failed_family_sample_field][other_sample_type_family_idx] is a list of sample_ids of variable length - it only contains failed sample IDs - so the [i] access won't work. I originally had failed_family_sample_field as an array of failing indices but switched it to an array of failing sample IDs because it was easier for me to come up with a solution to compare the same samples from different sample types.

Maybe a better way to structure failed_family_sample_field is an array of booleans that matches the shape of family_entries_field where the value at each sample index is true if failed or false if pass. It's just more complicated to compare samples individually that way - I'd need to reintroduce that map of families to samples to sample types to indices inside of their respective family_entries field.

[hanars]

I'd need to reintroduce that map of families to samples to sample types to indices inside of their respective family_entries field.

if failed_family_sample_field is a list of booleans then you can easily get the list of passing sample ids from the main family_entries field without maintaining an entry map, and then your logic for using other_sample_type_pass_samples would not need to change

hl.enumerate(ht[family_entries_field][family_idx]).starmap(
  lambda sample_idx, sample: hl.or_missing(
    ~ht[failed_family_sample_field][family_idx][sample_idx]),
    sample['sampleId'],
  )
).filter(hl.is_defined)

[jklugherz]

ok, I refactored the passes_inheritance_field to be a list of booleans corresponding to the samples in family entries, ^ and used this more performant code to get a list of passing sample IDs

[hanars]

I'd need to reintroduce that map of families to samples to sample types to indices inside of their respective family_entries field.

if failed_family_sample_field is a list of booleans then you can easily get the list of passing sample ids from the main family_entries field without maintaining an entry map, and then your logic for using other_sample_type_pass_samples would not need to change

hl.enumerate(ht[family_entries_field][family_idx]).starmap(
  lambda sample_idx, sample: hl.or_missing(
    ~ht[failed_family_sample_field][family_idx][sample_idx]),
    sample['sampleId'],
  )
).filter(hl.is_defined)

[hanars]

The search criteria changes between these 2 test cases. We really need back-to-back tests that the exact same search returns different results when the paternal data is present/absent. Instead of running a dominant and recessive search each filtered to variant 1 and variant 2, it would probably be better to not use an interval filter at all and test that dominant and recessive search return the expected combo of variants with and without paternal data

[jklugherz]

I updated this table to make variant2 de novo in WGS.

[hanars]

Commenting for posterity that I'm not 100% sure that this is the final logic we want, because it would allow us to return variants that pass quality and fail inheritance in one sample type and fail quality and pass inheritance in the other, meaning theres no sample type that clearly passes both inheritance and quality. However, I think we maybe do want to return these, the logic gets kind of confusing and I can't quite be sure these would not be helpful. I think being overly permissive here is better, if the analysts are seeing a bunch of cases where they ultimately think that the returned variants are not helpful and should be filtered out we can always go back later and make this a stricter criteria, so we should leave this as is for now

[jklugherz]

This makes sense, we may want to consider quality and inheritance passing together and handle it differently (the & seems like it could be too simple) but it's not clear to me what the logic/change would be. I agree that trying this out and getting feedback from analysts before we do that is the way to go.

[hanars]

I think this test file would be more readable if instead of toggling the inheritance back and forth you run both recessive searches back to back and then both de novo searches, so its clearer that nothings changing other than the parental data

[hanars]

this line is unneeded

[hanars]

we shouldn't need COMP_HET_ALL_PASS_FILTERS for de novo searches

[hanars]

I would clarify in the comment that inherited means "inherited and homozygous" as usually maternally inherited means mom has one alt allele and proband inherited that and has 1 alt allele

[hanars]

You updated the comment for variant 1 but not variant 2, de novo is usually a het call so its still confusing

[hanars]

this line is unneeded

[hanars]

this line is unneeded

[hanars]

You updated the comment for variant 1 but not variant 2, de novo is usually a het call so its still confusing

[hanars]

I still think a comment here explaining whats being tested is helpful. Maybe something like "Variant 2 fails inheritance when parental data is present"