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 filter location #3528

Merged
merged 12 commits into from
Aug 3, 2023
Merged

Hail backend filter location #3528

merged 12 commits into from
Aug 3, 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 16:53
Copy link
Contributor

@ShifaSZ ShifaSZ left a comment

Choose a reason for hiding this comment

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

It mostly looks good to me, except for one comment.

Comment on lines 126 to 129
@property
def should_add_chr_prefix(self):
reference_genome = hl.get_reference(self._genome_version)
return any(c.startswith('chr') for c in reference_genome.contigs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Using an attribute of the class and initializing it in the __init__ function performs better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no @property performs better because it will only be initialized if it is actually used which this isn't in lots of searches, but once its initialized the value is reused on every subsequent call and its not recomputed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind giving more context on why @property saves the recomputation? We might need @functools.cached_property to get the caching behavior (and I think exactly the behavior Shifa was asking for plus the benefit of it only being initialized if it's actually used).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh I think I misunderstood what the property was doing. I'll look into this

Copy link
Contributor

Choose a reason for hiding this comment

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

The property needs recomputation when self._genome_version is changed.

@hanars hanars changed the base branch from hail-backend-frequency-filter to dev August 2, 2023 16:42
@hanars hanars changed the base branch from dev to hail-backend-frequency-filter August 2, 2023 16:43
@hanars hanars requested a review from ShifaSZ August 2, 2023 16:54
@hanars hanars changed the base branch from hail-backend-frequency-filter to dev August 2, 2023 20:23
@hanars hanars merged commit 6483883 into dev Aug 3, 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