-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat!: add more support for queries in gnomad_vcf_to_protein #529
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.
This all seems correct to me.
@korikuzma we should consider asking @ahwagner to review as well to confirm. |
Since this endpoint needs gene-normalizer data, I'm wondering if we should use ProteinSequenceConsequence. Right now, I have the catvar pydantic models in metakb (it made development easier). Maybe we should move them here or in a different place? |
"""Define response for gnomad vcf to protein service""" | ||
|
||
gene_context: Optional[core_models.Gene] = None | ||
vrs_ref_allele_seq: Optional[StrictStr] = None |
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 could have just added this to variation extensions field, but thought it'd be easier for varcat if we had a field to retrieve
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 am always pro making varcat better (easier to develop). @ahwagner any objections?
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.
Yes, this is good info to include. I would like there to be a gene_context
field on the VRS 2.x SequenceReference
class but it doesn't exist yet, so we can leave that as a downstream issue.
Hmm. I will default to where @ahwagner thinks is best for them since he runs the whole show. |
* Updates cool-seq-tool + ga4gh.vrs versions. This mainly had to do with fixing inter-residue positions. cool-seq-tool previous versions returned 0-based coordinates. * Temporarily removes gnomad_vcf_to_protein, which will be added back in #529
@ahwagner Do you have time to review this? |
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 outstanding work. 👍
I noticed a few minor docstring clarifications and thread comments.
I spot tested a few of the provided tests, they check out.
"""Define response for gnomad vcf to protein service""" | ||
|
||
gene_context: Optional[core_models.Gene] = None | ||
vrs_ref_allele_seq: Optional[StrictStr] = None |
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.
Yes, this is good info to include. I would like there to be a gene_context
field on the VRS 2.x SequenceReference
class but it doesn't exist yet, so we can leave that as a downstream issue.
@korikuzma I am pretty sure I have reviewed this at least once if not more. Is there any reason for me to review again? |
Co-authored-by: Alex H. Wagner, PhD <[email protected]>
Co-authored-by: Alex H. Wagner, PhD <[email protected]>
@wesleygoar no, when changes get made the reviews are dismissed. I only re-request reviews when I need things to be reviewed again. |
Co-authored-by: Alex H. Wagner, PhD <[email protected]>
Co-authored-by: Alex H. Wagner, PhD <[email protected]>
Co-authored-by: Alex H. Wagner, PhD <[email protected]>
Co-authored-by: Alex H. Wagner, PhD <[email protected]>
close #362
@wesleygoar still in draft, but your feedback would be nice