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

Hail backend - In-silico filter #3530

Merged
merged 12 commits into from
Aug 4, 2023
Merged

Hail backend - In-silico filter #3530

merged 12 commits into from
Aug 4, 2023

Conversation

hanars
Copy link
Collaborator

@hanars hanars commented Jul 31, 2023

No description provided.

@hanars hanars requested review from bpblanken and ShifaSZ July 31, 2023 21:37
@hanars hanars changed the base branch from hail-backend-frequency-filter to dev August 2, 2023 20:56
@hanars hanars changed the base branch from dev to hail-backend-enum-setting August 2, 2023 20:56
require_score = in_silico_filters.get('requireScore', False)
in_silico_filters = {
k: v for k, v in in_silico_filters.items()
if k in self.PREDICTION_FIELDS_CONFIG and v is not None and len(v) != 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the v is not None and len(v) != 0 equivalent to v?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, this was copy-pasted from elasticsearch code, but it does not seem to be neccessary

@@ -411,6 +419,44 @@ def _filter_by_frequency(self, frequencies):
pop_filter &= pf
self._ht = self._ht.filter(hl.is_missing(pop_expr) | pop_filter)

def _filter_by_in_silico(self, in_silico_filters):
in_silico_filters = in_silico_filters or {}
require_score = in_silico_filters.get('requireScore', False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Using includes_missing_score for the requireScore is more understandable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but this is passed from the ui so its not something we can change. I think its clearer to name the variable what its called in the UI code and what its passed in the json as rather than to rename it to something else that means the same thing

@hanars hanars requested a review from ShifaSZ August 2, 2023 22:19
@hanars hanars changed the base branch from hail-backend-enum-setting to dev August 3, 2023 19:39
missing_q &= q
in_silico_qs.append(missing_q)

in_silico_q = in_silico_qs[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this block be replaced by hl.any(in_silico_qs)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Much cleaner 🎉!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feel like I was frustrated that there was no easy way to do this in hail, and now I'm like "oh duh, of course there is"

@hanars hanars merged commit a09284a into dev Aug 4, 2023
4 checks passed
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.

3 participants