-
Notifications
You must be signed in to change notification settings - Fork 62
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
Improve Beacon v2 (e111) #589
Conversation
@dglemos Hi, as we've already branched this repo for Release 110, please would it be possible to create a duplicate of this ticket to push against branch |
@nwillhoft, will do! |
Hi @nuno-agostinho, this is the Beacon PR. |
my ($gnomad_type, $pop) = $key =~ /(.*)_(.*)/; | ||
my $freq_obj; | ||
$freq_obj->{alleleFrequency} = $frequencies->{$key}; | ||
$freq_obj->{allele} = $allele; |
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.
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.
That's true. I added it because I think an allele is needed for each frequency in case there are multiple alt alleles.
I'm going to suggest an update for the Beacon schema.
@@ -950,11 +1040,19 @@ sub get_dataset_allele_response { | |||
|
|||
my @unique_genes = uniq @genes; | |||
$result_details->{MolecularAttributes}->{geneIds} = \@unique_genes if (scalar @unique_genes > 0); | |||
$result_details->{MolecularAttributes}->{geneOntology} = $gene_ontology if (scalar @{$gene_ontology} > 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.
geneOntology
is not part of the spec, is it okay to include it?
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 is another update I'm going to request https://github.com/ga4gh-beacon/beacon-v2
|
||
my @unique_molecular_effects = uniq @molecular_effects; | ||
$result_details->{MolecularAttributes}->{molecularEffects} = \@unique_molecular_effects if (scalar @unique_molecular_effects > 0); | ||
|
||
$result_details->{variantLevelData}->{clinicalInterpretations} = \@clinical; | ||
$result_details->{MolecularAttributes}->{molecularInteractions} = $molecular_interactions if (scalar keys %{$molecular_interactions} > 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.
molecularInteractions
is not part of the spec, is it okay to include it?
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 is another update I'm going to request https://github.com/ga4gh-beacon/beacon-v2
$disgenet_obj->{annotatedWith}->{toolName} = 'DisGeNET'; | ||
$disgenet_obj->{annotatedWith}->{version} = 'v7'; | ||
$disgenet_obj->{conditionId} = $disgenet->{diseaseName}; | ||
$disgenet_obj->{evidenceType} = 'PMID:' . $disgenet->{pmid}; |
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.
evidenceType
should have have an id
and a label
. They also recommend using the Evidence & Conclusion Ontology (ECO) terms: http://docs.genomebeacons.org/schemas-md/obj/evidenceType/
For a PubMed reference, I think the evidenceType
should have either:
id
IAO:000011 and a full reference including the PMID ID aslabel
id
isDefinedBy and just the PMID ID aslabel
@dglemos @nuno-agostinho Hi both, please could I check the status of this PR (just to get an idea of when this work can be merged into Release 110). Is there much work left to do on this? Thanks a lot. |
Hi @nwillhoft, PR is ready on my side. |
Hi @dglemos, am I correct in thinking that this PR is for Release 111 as we already merged in the previous Beacon v2 changes for Release 110? Thanks |
Yes, this PR is for release 111. |
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.
Test succeeded in the first place. Now, the test suite is failing because of version mismatch with the API (this repo hasn't branched yet).
Looks good to me
Description
Update Beacon endpoint to run VEP.
The idea is to return output for the following:
The Beacon schema does not specify all types of data we want to return so in this PR we return data for the following:
This change should also be included in release 110.
Some results are not included in the Beacon schema yet (example: pathogenicityPredictions). I created a ticket in the Beacon repo for that: ga4gh-beacon/beacon-v2#76
Use case
Benefits
Return vep data through Beacon, this way users can integrate VEP in Beacon.
Possible Drawbacks
None
Testing
note to submitters and reviewers: documentation-only changes may reflect
changes in other repos that can result in new or different output from
REST endpoints. In turn, these may require new tests or changes to existing tests.
Have you added/modified unit tests to test the changes?
If so, do the tests pass/fail?
Tests pass.
Have you run the entire test suite and no regression was detected?
No regression detected.
Changelog
[/ga4gh/beacon/query] Return VEP data in Beacon output