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

Updates to variant cooccurrence page #1335

Merged
merged 9 commits into from
Dec 5, 2023

Conversation

phildarnowsky-broad
Copy link
Contributor

This incorporates a number of updates to the variant cooccurrence page:

  • We change the thresholds at which a variant pair are considered to be in cis and in trans respectively
  • Singletons predicted to be in cis no longer get predictions, nor does the haplotype count table display for them
  • Variants that are in cis but that are distant from one another have an accuracy warning

There are also a good many refactorings added along the way, and some basic tests laid down.

Copy link
Contributor

@rileyhgrant rileyhgrant left a comment

Choose a reason for hiding this comment

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

LGTM! I like the pattern in the testing to define a base test object then change certain properties with the spreader syntax -- very cool!

Approved with minor changes of the usual variety.

  • One tiny comment on importing a constant value to reuse it
  • Linter should be appeased (which I see you're currently in the process of doing)
  • The DONTMERGE commit should be dropped, which I know you know, but I figure another friendly reminder (given its sufficiently friendly) almost never hurts

Nicely done as usual Phil :). I'm loving the tests.

Comment on lines +72 to +73
const cisThreshold = 0.02
const transThreshold = 0.55
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor comment, should we import these thresholds from VariantCoocurrencePage.tsx if they're being exported from that file anyways? One less thing to update if they ever change again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it's maybe a little counterintuitive, but when I have key constants like this which drive the logic, I like to define them separately for tests. The reason is that if you change the value in the future, a bunch of your tests will break, which will reveal some implications of changing the value that you need to think about.

@phildarnowsky-broad phildarnowsky-broad force-pushed the variant-cooccurrence-update branch from 41b268f to b68f9c9 Compare December 4, 2023 19:53
@phildarnowsky-broad phildarnowsky-broad force-pushed the variant-cooccurrence-update branch from b68f9c9 to 91dae95 Compare December 4, 2023 20:00
@phildarnowsky-broad phildarnowsky-broad merged commit 735a70e into main Dec 5, 2023
2 checks passed
@phildarnowsky-broad phildarnowsky-broad deleted the variant-cooccurrence-update branch December 5, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gnomad-browser variant coocurrence page and table changes
2 participants