Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix single source gen-anc frequencies on the Variant page #1547

Merged
merged 2 commits into from
May 28, 2024

Conversation

rileyhgrant
Copy link
Contributor

Resolves #1545

@rileyhgrant rileyhgrant self-assigned this May 17, 2024
@rileyhgrant rileyhgrant force-pushed the fix-variant-page-single-source-freqs branch 3 times, most recently from d41bdb4 to 5155926 Compare May 17, 2024 18:31
@rileyhgrant
Copy link
Contributor Author

Tagging you in for review, @ch-kr, in addition to Phil, because I wanted to check on one thing display decision wise.

The v4 exomes and genomes have identical genetic ancestry groups with the exception of ami, which is included presumably because the data was already there from v3 genomes. When both boxes are selected, or just the genomes box is selected, the table will include ami with the respective values there.

When the genomes checkbox is unselected, and only the v4 exome data is shown, should the ami row in the table disappear? The alternative would be to include a row for ami and populate it with AC: 0, AN: 0, AF: -

I'm not sure there's any value to showing it if it's always AC and AN of 0, but also including it keeps it more consistent.

Either is equally doable, I wanted to check on your opinion of what is more clear/preferable.

@rileyhgrant rileyhgrant requested a review from ch-kr May 17, 2024 18:37
@rileyhgrant rileyhgrant changed the title FIx single source gen-anc frequencies on the Variant page Fix single source gen-anc frequencies on the Variant page May 17, 2024
@ch-kr
Copy link
Contributor

ch-kr commented May 17, 2024

Thanks for the ping!

When the genomes checkbox is unselected, and only the v4 exome data is shown, should the ami row in the table disappear? The alternative would be to include a row for ami and populate it with AC: 0, AN: 0, AF: -

Quick question before I answer: based on this variant, it looks like the ami row currently exists for variants only present in the exomes. Does this question apply to these variants in addition to when a user explicitly selects or unselects the data type checkboxes?

@rileyhgrant
Copy link
Contributor Author

Thanks for the ping!

When the genomes checkbox is unselected, and only the v4 exome data is shown, should the ami row in the table disappear? The alternative would be to include a row for ami and populate it with AC: 0, AN: 0, AF: -

Quick question before I answer: based on this variant, it looks like the ami row currently exists for variants only present in the exomes. Does this question apply to these variants in addition to when a user explicitly selects or unselects the data type checkboxes?

Gah ok yeah I hadn't thought of that, good catch.

The question specifically was for behavior when the variant is present in both exomes and genomes.

Now I suppose there's another question of what to do for the variants that are not present in one data type, and thus don't allow de-selecting the type its not in. My initial thought there is leave it untouched, e.g. the example variant still includes the genome AN data? But also then if there's joint data maybe it makes sense to allow users to remove the ANs from the other data type that contribute to the overall AN per ancestry group.

@ch-kr
Copy link
Contributor

ch-kr commented May 17, 2024

thanks for clarifying! I think the most consistent behavior is to continue displaying the ami row both for variants only present in the exomes and for variants present in both data types (and where users can choose to unselect the genomes data). This reinforces that the v4 dataset is comprised of both exomes and genomes and also matches the behavior seen when toggling between the full dataset and a subset. For example, here's an example v3 variant -- the ami AC is 1 when all of v3 is selected and becomes 0 when the v3 non-TOPMed subset is selected.

On a slightly related note, this question sort of ties into this slack thread -- we should likely display the genetic ancestry group sample count/counts of each sex karyotype per group for v3/v4 on our stats page (across all groups, without any merged groups, which is what the table on the stats page currently contains)
image

@rileyhgrant rileyhgrant force-pushed the fix-variant-page-single-source-freqs branch from 5155926 to 88d9de9 Compare May 17, 2024 20:38
@rileyhgrant
Copy link
Contributor Author

thanks for clarifying! I think the most consistent behavior is to continue displaying the ami row both for variants only present in the exomes and for variants present in both data types (and where users can choose to unselect the genomes data). This reinforces that the v4 dataset is comprised of both exomes and genomes and also matches the behavior seen when toggling between the full dataset and a subset. For example, here's an example v3 variant -- the ami AC is 1 when all of v3 is selected and becomes 0 when the v3 non-TOPMed subset is selected.

On a slightly related note, this question sort of ties into this slack thread -- we should likely display the genetic ancestry group sample count/counts of each sex karyotype per group for v3/v4 on our stats page (across all groups, without any merged groups, which is what the table on the stats page currently contains) image

Perfect, we'll go with that behavior, thanks so much for the reply and opinion/decision.

@rileyhgrant
Copy link
Contributor Author

Hiya @phildarnowsky-broad

Tagging you in for review at your convenience. This fix is fairly impactful, and at least one user has written in about this unintended regression, commit by commit should (hopefully) be very bite sized.

Copy link
Contributor

@phildarnowsky-broad phildarnowsky-broad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small changes requested but at root this looks good

browser/src/VariantList/mergeExomeAndGenomeData.ts Outdated Show resolved Hide resolved
dataset-metadata/gnomadPopulations.ts Show resolved Hide resolved
dataset-metadata/metadata.ts Outdated Show resolved Hide resolved
@rileyhgrant
Copy link
Contributor Author

Alrighty, added some fixup commits to address comments from code review. Thanks, as always, for your review!

Re-requested you @phildarnowsky-broad

@rileyhgrant rileyhgrant force-pushed the fix-variant-page-single-source-freqs branch 4 times, most recently from a27d654 to 07752a6 Compare May 28, 2024 18:29
@rileyhgrant rileyhgrant force-pushed the fix-variant-page-single-source-freqs branch from 07752a6 to c87d3ee Compare May 28, 2024 18:31
@rileyhgrant rileyhgrant force-pushed the fix-variant-page-single-source-freqs branch from c87d3ee to e230458 Compare May 28, 2024 18:33
@rileyhgrant rileyhgrant merged commit 3730c8c into main May 28, 2024
5 checks passed
@rileyhgrant rileyhgrant deleted the fix-variant-page-single-source-freqs branch May 28, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

De-selecting a source on variant page does not update frequency information
3 participants