[builder] cellxgene_ontology_guide integration #1094
Conversation
| self._cached_tissues: dict[str, str] = {} | ||
| self._cached_labels: dict[str, str] = {} | ||
| self._uberon = owlready2.get_ontology(CXG_UBERON_ONTOLOGY_URL).load() | ||
| self.ontology_parser = OntologyParser() |
There was a problem hiding this comment.
you might want to pin to a particular schema version OntologyParser("v5.0.0") since the builder pins the version when it fetches the datasets from the index endpoint
There was a problem hiding this comment.
Does the version match the DP schema?
There was a problem hiding this comment.
yes, with a "v" prefix
| @@ -1,117 +1,16 @@ | |||
| """NOTE: In the future, this code will be part of an ontology service library.""" | |||
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1094 +/- ##
=======================================
Coverage 91.12% 91.12%
=======================================
Files 77 77
Lines 5913 5857 -56
=======================================
- Hits 5388 5337 -51
+ Misses 525 520 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| branch_ancestors = self._list_ancestors(is_a, branch_ancestors) | ||
|
|
||
| # Include this branch of ancestors is under anatomical structure | ||
| if self.ANATOMICAL_STRUCTURE_NAME in branch_ancestors: |
There was a problem hiding this comment.
Is this functionality covered by COG?
There was a problem hiding this comment.
this will not affect functionality. This is subsetting the ontology graph being built, but COG pre-builds the ontology graph and packages it in the library so it's not necessary, and is a superset of this graph. Census only matches terms to one of the terms in the curated list of tissue general terms anyway, so there isn't a concern around over-listing ancestors to users.
| elif self._is_entity(entity) and not self._is_and_object(entity): | ||
| # Entity is a superclass, check for is_a relationships | ||
|
|
||
| if entity.name in self.DENY_LIST: |
There was a problem hiding this comment.
COG doesn't have a deny list filter. Is this a problem?
There was a problem hiding this comment.
Most likely. I was expecting this function to be fully replaceable by get_high_level_terms, if it's not then I think we're not ready for using it in the Census.
There was a problem hiding this comment.
we already filter out non-UBERON terms. I don't foresee issues with the two UBERON terms in the deny list, since they are high-level terms closer to the root node than anything in the curated tissue general list, so skipping fetching their ancestors will not affect traversing the graph used for mapping to one of the curated tissue general terms. I believe this would only become an issue if a term higher-level than "material anatomical entity" or "anatomical entity" is added to the curated tissue list--which doesn't fit the use-case of the list, and further wouldn't make sense if the goal is actually to filter out ancestor terms of these terms
Are its functionalities covered by COG? This is only used by Geneformer, which isn't part of core Census, so I wouldn't be too worried about it. |
It's functionality is covered. I created a ticket to do this as future work. |
Co-authored-by: Trent Smith <[email protected]>
Integrate cellxgene_ontology_guide in place of the old TissueMapper.