-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add new variant search capabilities #73
Conversation
9ecbb99
to
3bdff7e
Compare
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.
Overall, this looks good. Would be awesome to have real GRCh38 support (like query coordinate liftover), but I think this PR satisfies the 80/20 rule.
The suggestion to refactor the code is not required for merging this (though it would be nice), but you should autodoc the search_variants_by_allele_registry_id
and search_variants_by_name
methods under a new docs subsection (maybe By Other Attribute
?) under Getting records
before we merge it in.
civicpy/civic.py
Outdated
def search_variants_by_allele_registry_id(caid): | ||
""" | ||
Search the cache for variants matching the queried Allele Registry ID (CAID) | ||
|
||
:param String caid: Allele Registry ID to query | ||
|
||
:return: Returns a list of variant hashes matching the Allele Registry ID | ||
""" | ||
variants = get_all_variants() | ||
return [v for v in variants if v.allele_registry_id == caid] | ||
|
||
def search_variants_by_name(name): | ||
""" | ||
Search the cache for variants matching the queried name | ||
|
||
:param String name: Variant name to query | ||
|
||
:return: Returns a list of variant hashes matching the name | ||
""" | ||
variants = get_all_variants() | ||
return [v for v in variants if v.name == name] |
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 think we should use this opportunity to create a generalized search_variants_by_attribute
(or, even better, search_class_by_attribute
) that mirrors this pattern, with appropriate params passed for the explicit routines. Will be less maintenance down the road.
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.
This has been implemented
ef0d0d0
to
a835ddb
Compare
@ahwagner I added the documentation for the two new search functions. I also added a catch guard to the bulk coordinate search since it doesn't currently support search by non-GRCh37 builds. |
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.
Looks great. Merge at your leisure.
This PR adds variant search by:
TODO:
Add tests