obophenotype / cell-ontology

An ontology of cell types
https://obophenotype.github.io/cell-ontology/
Creative Commons Attribution 4.0 International
142 stars 49 forks source link

Massive amount of unsats in cl-edit #1782

Closed shawntanzk closed 1 year ago

shawntanzk commented 1 year ago

@bvarner-ebi spotted that there are a lot of unsats in cl-edit.owl in master branch interestingly explain_unsat isn't picking this up which is probably why QC didnt pick up on it Here's an example from @bvarner-ebi

209308227-51e3ec28-f9e2-44d1-87c9-edcc751fbc0a

@bvarner-ebi suspects this PR in Ubeorn https://github.com/obophenotype/uberon/pull/2695 (tagging @gouttegd) Will try to look into this when I can, but any help is also welcomed

gouttegd commented 1 year ago

The (main? only? not sure yet) culprit seems to be results in developmental progression of (RO:0002295), which has a range restricted to connected anatomical structure (CARO:0000003) (which is itself DisjointWith disconnected anatomical group, CARO:0020000).

This restriction seems wrong. A developmental process can very well result in the development of an entity made of disconnected parts, not only of a connected structure. E.g., endocrine system development results in the development of the endocrine system, which is a non-connected functional system (it is made of endocrine glands dispersed throughout the body).

This range restriction should either be removed or at least replaced by a restriction to a higher class in the hierarchy, one that does not imply anything about “connectedness” (anatomical entity would be correct, I think).

ghost commented 1 year ago

Thank you for taking a look, @gouttegd.

Reviewing related tickets I found this comment, where it was proposed that 'anatomical cluster' should be a 'disconnected anatomical group'. I'm not sure this is accurate, but unsure how "connected" is defined.

gouttegd commented 1 year ago

I note that the definition of RO:0002295 does state that the relation applies (or should apply) to anatomical structures:

p results in the developmental progression of s iff p is a developmental process and s is an anatomical structure and p causes s to undergo a change in state at some point along its natural developmental cycle (this cycle starts with its formation, through the mature structure, and ends with its loss).

The range restriction is consistent with that definition, but then, if we follow that, the relation should not be used to describe the development of anatomical entities that are not anatomical structures. And again that seems wrong to me: I see no reason for limiting the use of that relation to the development of anatomical structures, especially since there are no equivalent relations to describe the development of disconnected anatomical entities.

gouttegd commented 1 year ago

Reviewing related tickets I found https://github.com/obophenotype/uberon/issues/2682#issue-1420638803, where it was proposed that 'anatomical cluster' should be a 'disconnected anatomical group'. I'm not sure this is accurate, but unsure how "connected" is defined.

I am not 100% sure either. As I wrote in that comment, it depends on how you interpret “adjacents” in the definition of anatomical cluster (“anatomical group that has its parts adjacent to one another”). I took it as not implying that the parts are connected, that is, they don’t have physical connection to each other.

(For reference, PATO defines disconnected as “a structural quality inhering in the bearer by virtue of the bearer consisting of multiple structures lacking any physical connection to each other”.)

ghost commented 1 year ago

I took it as not implying that the parts are connected, that is, they don’t have physical connection to each other.

(For reference, PATO defines disconnected as “a structural quality inhering in the bearer by virtue of the bearer consisting of multiple structures lacking any physical connection to each other”.)

If parts are adjacent (which I interpret as sharing a physical boundary), I would argue that they are physically connected.

gouttegd commented 1 year ago

I won’t argue with that, feel free to revert that change if you want. (Please do let me know if you do, because that term is mapped to FBbt’s anatomical cluster, which is clearly defined as disconnected; if you re-classify it as connected, I will need to remove the mapping.)

I actually question the usefulness of anatomical cluster in Uberon, regardless of its definition, given the diversity of its subclasses: muscle spindle, pineal complex, upper urinary tract, vasculature … Seems like a mess to me: under which definition would the vasculature or the urinary tract be anatomical clusters ?

dosumis commented 1 year ago

Anatomical cluster in fbbt does useful work. The uberon term inherits an incoherent mess from FMA. Fully agree with Damien that this should be cleaned up. If align the def with fbbt and remove most subclasses.

Also makes sense to relax domain/range on the dev relation causing unsats.

On Fri, 23 Dec 2022, 12:14 Damien Goutte-Gattat, @.***> wrote:

I won’t argue with that, feel free to revert that change if you want. (Please do let me know if you do, because that term is mapped to FBbt’s anatomical cluster, which is clearly defined as disconnected; if you re-classify it as connected, I will need to remove the mapping.)

I actually question the usefulness of anatomical cluster in Uberon, regardless of its definition, given the diversity of its subclasses: muscle spindle, pineal complex, upper urinary tract, vasculature … Seems like a mess to me: under which definition would the vasculature or the urinary tract be anatomical clusters ?

— Reply to this email directly, view it on GitHub https://github.com/obophenotype/cell-ontology/issues/1782#issuecomment-1363904907, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA3RR6VD7J6M2VMKQQGTILWOWJSNANCNFSM6AAAAAATHRRLQ4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

ghost commented 1 year ago

I actually question the usefulness of anatomical cluster in Uberon, regardless of its definition, given the diversity of its subclasses: muscle spindle, pineal complex, upper urinary tract, vasculature … Seems like a mess to me: under which definition would the vasculature or the urinary tract be anatomical clusters ?

Agreed. I cannot justify why these terms are 'anatomical clusters'.

Of note, removing the range 'connected anatomical structure (CARO:0000003)' from 'developmental progression of (RO:0002295)' fixes all unsats but one (a GO term: 'calcium activated cation channel activity').

ghost commented 1 year ago

Also makes sense to relax domain/range on the dev relation causing unsats.

@dosumis, should this change and new CL release wait until the new year?

gouttegd commented 1 year ago

all unsats but one (a GO term: 'calcium activated cation channel activity').

This one seems to be caused by regulated by (RO:0002334), which has both a domain and a range restriction to process (BFO:0000015) (so, a process is regulated by another process), where process is an occurrent.

calcium activated cation channel activity is defined as gated channel activity (a process, OK) that is regulated by some calcium(2+) (not a process).

shawntanzk commented 1 year ago

calcium activated cation channel activity is defined as gated channel activity (a process, OK) that is regulated by some calcium(2+) (not a process).

I think this might be a RO issue, again I think the range needs to be relaxed too, given a process can be regulated by things other than a process (probably also means we should change the domain of regulates given that regulates by is defined by regulates)

gouttegd commented 1 year ago

@shawntanzk Not sure about that. regulated by is a subclass of causal relation between processes, so the range and domain restrictions are clearly not accidental – the relation is explicitly intended to be about processes on both ends. There is another set of relations intended for causal relations between a process on one side and a material entity on another side (RO:0002595 and its subclasses), the definition of calcium activated cation channel activity should probably use one of those instead.

dosumis commented 1 year ago

calcium activated cation channel activity is defined as gated channel activity (a process, OK) that is regulated by some calcium(2+)

@gouttegd is correct - this is a GO error. GO should fix by changing to regulated by 'Ca2+ binding'.

ghost commented 1 year ago

To revisit this issue, is the ideal approach as follows:

1: In CL, remove the range 'connected anatomical structure (CARO:0000003)' from 'developmental progression of (RO:0002295)' 2: Resolve https://github.com/geneontology/go-ontology/issues/24605 3: Refresh CL imports and make a new CL release

For step 1, does this change need to happen elsewhere / outside of CL? For step 2, can someone recommend a GO editor to ping?

Thanks again for the help with this.

gouttegd commented 1 year ago

Step 1 should happen in obo-relations. There’s already a PR for that (oborel/obo-relations#659), once it’s merged (only after a discussion about it at the next RO meeting apparently, whenever that meeting may be) the fix will be brought to CL by refreshing the RO import.

shawntanzk commented 1 year ago

whenever that meeting may be

luckily it is today :) I'll attend and try to make sure this gets merged in (don't think its super controversial). Once that happens, I'll try to do a release (if merged in this evening, will probably get it done by tmr).

gouttegd commented 1 year ago

don't think its super controversial

Famous last words… :D

shawntanzk commented 1 year ago

For step 2, can someone recommend a GO editor to ping?

@raymond91125 was wondering if you could help with this (see https://github.com/geneontology/go-ontology/issues/24605) if not could you suggest someone that can help us with this please? thanks heaps :)

shawntanzk commented 1 year ago

GO issue should be solved in this PR https://github.com/geneontology/go-ontology/pull/24610 (see this ticket https://github.com/geneontology/go-ontology/issues/24605), will probably just have to refresh import after next GO release :)

shawntanzk commented 1 year ago

RO issue solved too :D will make a RO release tmr -> want to wait till after RO meeting today to let anyone get their stuff in and all

ghost commented 1 year ago

With the new CL release, the only unsatisfiable class is now GO:0005227 'calcium activated cation channel activity'. The GO term update appears to have been merged. This last unsatisfiable class should be resolved once there is a new GO release and it is imported to CL.

ghost commented 1 year ago

Now that GO has a new release release and imports have been refreshed in CL, there appear to be no more unsats in cl-edit.

Thanks to all for the help.