Conversation
Add a menu item, along with the corresponding support in the ClassHierarchyPreferences, to display only the classes below the ontology roots (if any) in the class hierarchy.
Even though the AssertedClassHierarchyProvider has a public `getRoots()` method returning a set of OWL classes (suggesting there can be several roots), the private code of the provider assumes that there can be only one root (`owl:Thing`). This will no longer always be the case when the "display from ontology roots" option will be enabled, because an ontology may very well declare more than one root (Uberon, for example, declares two roots). So we must update the hierarchy provider so that it is ready to deal with a _set_ of roots instead of a single root.
Add a `setDisplayFromOntologyRoots(boolean)` method to the class hierarchy provider. If called with true, it will attempt to find self-declared roots in the ontology sets (declared with the IAO:0000700 annotation) and, in the presence of such roots, will use them as the roots of the hierarchy. All the DisplayHierarchyFromOntologyRootsAction then has to do is to call this method whenever the action is performed. closes #1184
|
confirming this is working as expected for HGNC before: 
after: 
|
|
I found some unexpected/surprising behavior. I was browsing a gene, then tried to navigate to a class that was declared as a child of something that isn't in any of the hierarchies of the roots, then I get an unexpected window e.g., in HGNC, navigate to any gene, then click on the taxon (homo sapiens) or the chromosome in the "subclass of" logical axioms. Both the Homo sapiens class and all of the chromosome classes are not under any of the roots. 
then, it seems to get confused and doesn't show anything 
if I turn off the "Display from ontology roots" then the behavior returns to normal |
|
From @matentzn :
I confirm the issue, and that’s definitely a bug. |
Whenever the set of ontologies used by the ClassHierarchyProvider is changed, the current set of roots should be refreshed, because we cannot expect that the new set will always have the same roots as the previous set -- that was the case previously when the only possible root was `owl:Thing`, but now that we allow ontologies to declare their own roots, the roots may be ontology-dependent. Also fix a typo on the name of the `setDisplayFromOntologyRoots` method.
matentzn
left a comment
matentzn
left a comment
There was a problem hiding this comment.
I think the implementation looks great. My main concern at the moment (which is sort of also my problem why I did not ship faster) is thread safety - do we need to make the roots and displayFromOntologyRoots variables volatile so that all threads can see the variable in the exact same state at each time, or is this not a relevant issue for Protege architecture?
`getOntologyRoots` does not, in fact, return the roots -- it fills the given set object and returns a boolean indicating whether it found self-declared roots or not. So we rename it to `findOntologyRoots` to avoid needless confusion.
The `getRoots()` method used to return an _unmodifiable_ set containing the roots used by the class hierarchy provider. It should still be doing that. To be clear: this is _not_ about thread-safety. This is about preserving the existing contract of the `getRoots()` method. Callers of that method expected to get an unmodifiable copy of the set of roots, so that's what they should still get.
Wrap all the code within `setDisplayFromOntologyRoots` within a write lock, to ensure that any thread trying to access either the `roots` set or the `displayFromOntologyRoots` variable (from elsewhere in the class hierarchy provider, where such access are already wrapped within a read lock) would be blocked until we are done finding the roots.
|
Closed in favour of #1332 |
3 participants
This PR adds support for ”self-declared” ontology roots: when an ontology contains
http://purl.obolibrary.org/obo/IAO_0000700annotations, the classes pointed by these annotations are used as the roots of the class hierarchy, instead ofowl:Thing.closes #1184