-
Notifications
You must be signed in to change notification settings - Fork 93
Conversation
I don't see any documentation, but it is incorrect to say this is adding an ontology. it's adding a mapping between to an term coming from a VCF and a reference to an ontology term (OntologyTerm) record. This is only a problem with the terminology, not the code. But that is very important, because it claims |
@@ -104,7 +104,12 @@ def getOntology(self, name): | |||
""" | |||
Returns ontologies | |||
""" | |||
return self._ontologyNameMap[name] | |||
ret = {} | |||
try: |
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'd remove the try/except and instead do:
ret = self._ontologyNameMap.get(name, {})
You almost never want to write except: pass
... kinda defeats the point of exceptions
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.
note that both of these approaches create an dict object that is then discarded even when the name map contains the object. This is inefficient.
A good start. Also should have tests for the individual commands, including failure cases, in |
60980b9
to
a132cec
Compare
425fe70
to
7a374cc
Compare
Updated @dcolligan. They are now more appropriately called ontology maps. |
95dc486
to
da31e3e
Compare
self.ontologyMapName = args.ontologyMapName | ||
|
||
def run(self): | ||
self.repoManager.removeOntologyMap(self.ontologyMapName) |
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 needs to use confirmRun
or else the force flag does nothing
It's coming along. After the changes I mentioned, you should be good to go. |
d208847
to
0015896
Compare
@@ -27,10 +27,20 @@ def add(self, id_, name): | |||
self._idNameMap[name] = id_ | |||
|
|||
def getId(self, name): | |||
try: |
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 seems problematic. Usually failing noisily is a good thing. We should provide methods like hasId
and hasName
for the cases when we want to know if a given id/name exists in the map.
Looks good in general to me. |
You assume id<->name will be 1:1 within any given ontology. For the id->name direction this is not particularly problematic. Every id uniquely identifies an ontology class, and within the subset of the ontologies you are likely to use, there should be a single rdfs:label. However, this assumption cannot be made for every ontology ever constructed. You might also want to think about internationalization early on. For name->id, for any well-behaved ontology this should be OK, provided you are only mapping non-obsolete classes. It's not clear what procedure you are using to generate the mapping tables, and if overwrites are happening here. I recommend documenting these assumptions, and the set of ontologies for which the assumptions are assumed to hold (I can help set up upstream checks in the release pipelines of these ontologies). You're also using strings like I expect you will need a richer ontology model at some point, at least with support for synonyms, traversal, definitions, obsoletion metadata, versioning. Ping me if you want feedback on either requirements or code. (none of this constitutes a downvote, code looks a great start, feel free to ask me to move my comments to a separate ticket etc) |
Thanks @cmungall note that the mapping goal is to link existing data that only The goal is not to develop an ontology service, only to Chris Mungall notifications@github.com writes:
|
1fc3992
to
4c7130e
Compare
Added the hasId, hasName functions for future use and added ontology maps to repo Now throw an exception when a key isn't found that has to be caught elsewhere. |
Could I get a review/merge? |
@@ -994,7 +994,10 @@ def convertSeqOntology(self, seqOntStr): | |||
so = self._createGaOntologyTermSo() | |||
so.term = soName | |||
if self._sequenceOntology is not None: | |||
so.id = self._sequenceOntology.getId(soName) | |||
try: |
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.
Is this really the semantics we want (using the empty string when the key isn't defined)? If so, how about we change the getId
function to follow the semantics of dict.get
, and add a second parameter to give the default value when the key is not found? So, here we'd have so.id = self._sequenceOntology.getId(soName, "")
Change ontology to ontology maps Update prepare compliance data Add some documentation Confirm run and remove tests Gracefully fail if the id or name is not found
Raise a named exception when lookup fails Add functions for testing for presence of ID or name
1ae677a
to
34a0572
Compare
|
||
def getName(self, id_): | ||
return self._nameIdMap[id_] | ||
def getId(self, name, default=""): |
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 function can be rewritten
return self._idNameMap.get(name, default)
One is able to provide the default value of getting by name or ID similar to python dictionary Improve add-remove test
34a0572
to
0a01c47
Compare
Thanks @dcolligan got it. |
Most of this the changes here are in the repo manager, so from the perspective of the other parts of the server's datamodel, the only difference is that I would now call (for example) |
+1 |
1 similar comment
+1 |
VA and SA both use sequence ontology. This adds the ability to add and remove ontologies using the repo manager.