-
Notifications
You must be signed in to change notification settings - Fork 29
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
Modifications to annotate_freq
to improve memory use
#577
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this. The logic all looks great and this is way cleaner on top of improving performance. I just have a couple suggestions and questions.
gnomad/utils/annotations.py
Outdated
freq_expr = freq_expr.map( | ||
lambda cs: cs.annotate( | ||
AC=cs.AC[1], | ||
# TODO: This is NA in case AC and AN are 0 -- should we set it to 0? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we address this here? This TODO has been in here for a while. We could ask in a channel what the preference is for the HT. The browser displays 0 when AN is 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, can you initiate the conversation?
Co-authored-by: Mike Wilson <mwilson@broadinstitute.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
No description provided.