Add hemizygous count to structural variant tables #1566
Conversation
|
fixes #1462 |
| @@ -29,6 +29,7 @@ const StructuralVariantsInGene = ({ datasetId, gene, zoomRegion, ...rest }: Prop | |||
| gene(gene_id: $geneId, reference_genome: $referenceGenome) { | |||
| structural_variants(dataset: $datasetId) { | |||
| ac | |||
| ac_hemi | |||
There was a problem hiding this comment.
Nitpick: I think the formatter doesn't run due to our noted strange practicing of having a big ol' string that represents the query -- I'd prefer consistent indentation here but fully realize this is nitpicky and this is non-blocking
There was a problem hiding this comment.
@rileyhgrant this looks like it's aligned correctly now--I did see, earlier, some odd discrepancies between the diffs and my editor. not sure what to make of all this but I will clean it up if there is anything to clean up.
There was a problem hiding this comment.
looks like a tabs-vs-spaces issue maybe, unclear how a tab got in there in the first place
| @@ -30,6 +30,7 @@ const StructuralVariantsInRegion = ({ datasetId, region, zoomRegion, ...rest }: | |||
| region(chrom: $chrom, start: $start, stop: $stop, reference_genome: $referenceGenome) { | |||
| structural_variants(dataset: $datasetId) { | |||
| ac | |||
| ac_hemi | |||
There was a problem hiding this comment.
Same as other comment, consistent indentation here would be nice, but is not strictly neccesary
| contextNotes: 'Not shown when viewing Y chromosome', | ||
| minWidth: 100, | ||
| compareFunction: makeNumericCompareFunction('ac_hemi'), | ||
| render: (variant: any) => renderAlleleCountCell(variant, 'ac_hemi'), | ||
| shouldShowInContext: (context: Context) => context.chrom !== 'Y', |
There was a problem hiding this comment.
These attributes for the context dependent notes and whether or not to show in the context appear to be duplicated from the above column (homozygote count) when I think they should be different.
For the short variants, this column has a note that it only appears for the sex chromosomes, and logic to achieve this.
From variantTableColumns.tsx:
{
key: 'hemizygote_count',
heading: 'Number of Hemizygotes',
description: 'Number of individuals hemizygous for alternate allele',
contextNotes: 'Only shown when viewing X or Y chromosomes',
grow: 0,
minWidth: 100,
compareFunction: makeNumericCompareFunction('ac_hemi'),
render: (variant: Variant) => renderAlleleCountCell(variant, 'ac_hemi'),
shouldShowInContext: (context: any) => context.chrom === 'X' || context.chrom === 'Y',
},
I think we should at least mimic this behavior to stay consistent, this may also be an opportunity to re-use this column definition, rather than duplicating it.
There was a problem hiding this comment.
I also think we should add this column to the default columns rendered for SVs, and let the individual column definition logic handle whether or not its rendered.
The default columns are defined in StructuralVariants.tsx, on line 49:
const DEFAULT_COLUMNS = [
'source',
'consequence',
'class',
'pos',
'length',
'ac',
'an',
'af',
'homozygote_count'
]
We could add hemizygote_count to the above list to achieve this.
There was a problem hiding this comment.
@rileyhgrant it's tempting to me to go through these lists of column specifiers and look for more copypasta to extract, but in the absence of tests and presence of other things I have to do, I'm leaving that for another day.
There was a problem hiding this comment.
@rileyhgrant ...actually, the two definitions of hemizygote_count are similar but not compatible, I'm just going to leave them separate and fix up the new SV version and call it a day until such time as I have the wherewithal to really overhaul this code.
phildarnowsky-broad
force-pushed
the
hemizygote_count_in_sv
branch
from
e62c652 to
f121a7c
Compare
|
@rileyhgrant ready for another gander |
phildarnowsky-broad
force-pushed
the
hemizygote_count_in_sv
branch
from
8600808 to
bf8fe2b
Compare
Per #1462, the hemizygous AC was available in short variant but not structural variant tables. This adds that field to SV tables.
Note that I'm breaking my own first rule here and not adding test coverage: these components turn out to be coupled together with some other logic in a way that makes writing a coherent test for them a real pain. I don't love doing this but this is a very basic change that fixes something the end-user already needs, and it's not really clear to me yet how to best refactor this to make testing it less of a circus.