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

Split variant flags #1623

Merged
merged 4 commits into from
Sep 11, 2024
Merged

Split variant flags #1623

merged 4 commits into from
Sep 11, 2024

Conversation

phildarnowsky-broad
Copy link
Contributor

This updates the API and frontend to support flags both on a variant generally, or on the exome and/or genome specifically, and fixes a number of related bugs related to display of flags.

fixes #1614

@phildarnowsky-broad
Copy link
Contributor Author

@rileyhgrant I've decided to put the VA/VRS-related fixes in the other VA topic branch I've been working on, which I'll rebase on top of this, hence this PR doesn't include VA fixes.

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, very nicely done, I especially like the care taken to the testing, and to a neatly organized series of commits.

Just one minor comment about the naming of a common region flag (based on the docs, it might be named "non_par", rather than "par", but I wasn't quite sure and wanted to double check).

Nice one!

const baseExomeFlags = variant.exome?.flags || []
const baseGenomeFlags = variant.genome?.flags || []

const regionalFlags = ['par', 'segdup', 'lcr'].filter(
Copy link
Contributor

@rileyhgrant rileyhgrant Sep 10, 2024

Choose a reason for hiding this comment

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

Based on the hail tables help text, it appears as though the par flag might be named "non_par"?

I did a quick grep and didn't see any place where we rename this flag in our pipeline, so just wanted to check, this flag seems to potentially differ in meaning from in the SV datasets where there is a "par" flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The v4 data pipeline does address this,

, but it would be smart for me to double-check that it's correct.

Copy link
Contributor

@rileyhgrant rileyhgrant Sep 10, 2024

Choose a reason for hiding this comment

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

Ah whoops, I naively only grepped for "non_par", my bad for not finding that, and nice catch!

@phildarnowsky-broad phildarnowsky-broad merged commit fa15c6a into main Sep 11, 2024
5 checks passed
@phildarnowsky-broad phildarnowsky-broad deleted the split_variant_flags branch September 11, 2024 15:12
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.

Fix variant flag access based on now-deleted variant.flags field
2 participants