search

ga4gh / ga4gh-schemas

Models and APIs for Genomic data. RETIRED 2018-01-24
http://ga4gh.org
Apache License 2.0
214 stars 114 forks source link

Change ontology term identifier #694

Closed david4096 closed 7 years ago

david4096 commented 8 years ago

Ontology terms had an ambigious 'id' field and description. This PR renames the field to term_id to remove the confusion over the role of the field, as well as improves the comment for its proper use. A field 'term_uri' has been added that allows one to specify the full URI for a term if it is available.

Closes #621 #165

mbaudis commented 8 years ago

+1 Clear improvement.

sarahhunt commented 8 years ago

+1

mbaudis commented 8 years ago

@david4096 (pls. don't forget the docs)

david4096 commented 8 years ago

Thanks @mbaudis updated with some minor tweaks to the ontology documentation.

david4096 commented 8 years ago

Related to discussion in #165

mcourtot commented 8 years ago

+1 with small modification (with input from @cmungall): "if no CURIE are available." -> "if no URI available".

david4096 commented 8 years ago

Thanks @mcourtot! Updated!

mbaudis commented 8 years ago

Please check termId => term_id (if I understand the proto snake_case correctly); same for termUri => term_uri. I usually do things like that with serarch/replace over the whole tree; but don't tell anybody, please.

If fixed -> merge.

david4096 commented 8 years ago

Thanks @mbaudis good to go!

david4096 commented 8 years ago

Thanks everyone for your input @jmcmurry @mcourtot @drseb ! I'll be happy to integrate any further work on this data model as consensus is reached.

For now, I'd like to see if we can move ahead and take further discussion to another issue. This PR was designed to improve the situation from id to term_id and improve the comments, which it has done. To continue the discussion check out the below links:

Ontology Source Identification Multiple Ontology Term URIs OntologyTerm.id definition

jmcmurry commented 8 years ago

That sounds good to me; thanks @david4096

cmungall commented 7 years ago

And -10 to any date scheme that is not ISO-8601!

mbaudis commented 7 years ago

@cmungall @drseb ... but reminding me of https://github.com/phenopackets/phenopacket-format/issues/75

mbaudis commented 7 years ago

@mcourtot @cmungall @david4096 This should now be updated to reflect the Vancouver discussions.

mcourtot commented 7 years ago

Hm - I tried and push a commit to the pr/694 branch to reflect the Vancouver changes but don't see them here. Hadn't tried and commit over a PR before, so maybe i'm looking at the wrong place?

david4096 commented 7 years ago

@mbaudis @mcourtot I've incorporated your changes from https://github.com/ga4gh/schemas/pull/726 .

Since we have removed the URLs, I believe we need a way to manage them, or at least specify a place where they might be looked up. What do you think @cmungall ?

mcourtot commented 7 years ago

Thanks @david4096! We're planning on using OLS, http://www.ebi.ac.uk/ols/index to bind prefixes to their URIs. @simonjupp, @tburdett, do you have a specific link?

david4096 commented 7 years ago

Implemented in the server, client, and compliance.