Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add new absent terms #332

Merged
merged 12 commits into from
Aug 9, 2022
Merged

add new absent terms #332

merged 12 commits into from
Aug 9, 2022

Conversation

nicolevasilevsky
Copy link
Contributor

addresses #331

@dosumis dosumis requested a review from cmungall July 31, 2020 08:34
@dosumis
Copy link
Contributor

dosumis commented Jul 31, 2020

@balhoff - can you take a look at this? I'd like to add you as a reviewer, but you may need to accept editor's rights first.

Copy link
Contributor

@dosumis dosumis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like some review by key people who weren't at the meeting. Particularly @balhoff

src/ontology/pato-edit.obo Outdated Show resolved Hide resolved
src/ontology/pato-edit.obo Outdated Show resolved Hide resolved
src/ontology/pato-edit.obo Outdated Show resolved Hide resolved
src/ontology/pato-edit.obo Outdated Show resolved Hide resolved
@sbello sbello requested a review from ybradford July 31, 2020 14:01
@sbello
Copy link
Contributor

sbello commented Jul 31, 2020

I've added Yvonne to the review list as well.

@balhoff
Copy link
Contributor

balhoff commented Aug 2, 2020

What are these qualities of? Is this using the same EQ model as some other quality? pointed and inheres_in some ear vs. 'absent anatomical entity' and inheres_in some ear? Or did you come up with another model?

@dosumis
Copy link
Contributor

dosumis commented Aug 27, 2020

@balhoff - I'm afraid it's just the same old problematic patterns, extended to ensure that absent anatomical entity phenotypes end up under abnormal morphology phenotypes for that entity and - which fits with editor expectations.

TBH - I still prefer your solution here: https://arxiv.org/pdf/1410.3862.pdf (I strongly recommend that everyone following this pull request ant the associated ticket read it). It's the only one that is comprehensive and consistent (in the general sense as well as the logical sense). But we're still stuck trying to cope with irreconcilable expectations about classification and term interpretation. The main clash is:

  • A. Expectation that absent X phenotypes should classify under general X phenotypes e.g. absent limb classifies under abnormal limb (at a minimum) but ideally under abnormal limb morphology, perhaps also under reduced limb size (limit point of size reduction is absence).

VS

  • B. Expectation that existential assertions about some entity imply its presence (basic logic) - something your paper solves elegantly

I don't think there's any way to reconcile these.

A requires 'absent X' to be interpreted as something like 'absence of some (type of ) X" That way inferences like this make sense:

absent tooth (the singular helps interpretation here here)
. < - SubClassOf absent canine tooth

BUT... This doesn't fit editor expectations either. Here's MP (MGI browser - no inference - showing intention)

image

Here's the same (with inference - OLS):

image

Using punning and value restriction would (as in https://arxiv.org/pdf/1410.3862.pdf) would fix this, but also lose the inferred placement under abnormal tooth morphology. We need this inference (& similar) for classify species neutral classes in uPheno2 and Phenotypes in XPO, both of which have no asserted manual classification (see XPO example here obophenotype/xenopus-phenotype-ontology#125)

@matentzn
Copy link
Contributor

One way around this may be to use non logical modelling, like skos:broadMatch or something along these lines to cover "expectations about classification" and help groups that use it to implement it into their hierarchical browsers..

@dosumis
Copy link
Contributor

dosumis commented Aug 27, 2020

One possible compromise:

Define 'abnormal x phenotype' as disjointUnionOf: 'abnormal x morphology' and 'absent x'. We'd have to give up on nesting absence of X under abnormal morphology of X, but we would have grouping classes at the top level - with abnormal X phenotype terms. This works nicely with HermiT, but DisjointUnion of is outside of EL. However, I think we could modularise to infer (or even have some bespoke assertion job) => classification immediately under these top level terms

@dosumis
Copy link
Contributor

dosumis commented Aug 28, 2020

We can still get terms into the morphology branch, just under the morphology for parent structure in the partonomy.

absent femur: 'lacks all parts of type' and (inheres_in some hindlimb) and (towards value femur)

'lacks all parts of type' SubPropertyOf morphology

hindlimb morphology phenotype: morphology and inheres_in some hindlimb

=>
image

Demo file here: https://gist.githubusercontent.com/dosumis/c6de453407469a3d517d955efa96478b/raw/9dfaf6b5acb9b687b502669733d282c2fdb6d541/presence_absence_morphology.owl
(derived from http://purl.org/phenoscape/demo/presence_absence.owl - which is the demo ontology from Jim's paper, linked above)

@nicolevasilevsky
Copy link
Contributor Author

@dosumis @matentzn @balhoff is there agreement on the approach needed here? Should we close this PR or are there steps to move this forward?
thanks!

@sbello
Copy link
Contributor

sbello commented Jul 22, 2022

I'll add this to the agenda for the next UPheno call. I think the changes look good but I was at the meeting. I think the group should review and after the call we can decide if we want to stick with this approach.

@nicolevasilevsky
Copy link
Contributor Author

nicolevasilevsky commented Jul 22, 2022

great, thanks @shawntanzk! I'll hold off on fixing the conflict and broken QC until it is confirmed that we want to move forward.

@sbello
Copy link
Contributor

sbello commented Jul 28, 2022

Discussed on the UPheno call 7/28
suggest that absence of anatomical entity be a child of absence of physical object as well as a child of morphology

this PR seems to solve the morphology - physiology divide but not the absent some vs absent all issue
Is this intended to solve both issues?

Suggest we add a curatorial note indicating that absent should only be used when all of the entity is absent. eg absent teeth would mean all teeth are absent not some teeth are absent.

Copy link
Contributor

@sbello sbello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see earlier comment for suggested changes from 7/28/22 call

@nicolevasilevsky
Copy link
Contributor Author

Thanks @sbello, I'll work on this :)

@nicolevasilevsky
Copy link
Contributor Author

nicolevasilevsky commented Aug 3, 2022

  • assigned new ID to absence of anatomical entity b/c another term is now using the former ID (new ID = PATO:0040060)
  • assigned new ID to absence of physical object b/c another term is now using the former ID (new ID = PATO:0040058)
  • assigned new ID to absent process b/c another term is now using the former ID (new ID = PATO:0040059)
  • added parent to absence of anatomical entity: absence of physical object

@sbello I think these changes address all of the action items that were assigned to me in #331. There is still more work needed to address this entire ticket. And I don't think this addresses the question of absent some vs absent all

@nicolevasilevsky
Copy link
Contributor Author

nicolevasilevsky commented Aug 3, 2022

  1. @sbello should I announce that we will obsolete ‘absent’ and will switch to using the new terms?
  2. Could you create a new ticket for the absent some vs absent all issue?
  3. I'll follow up with Chris Grove about updating the patterns after this is merged
  4. I'll work with Nico on fixing the imports after this is merged

@nicolevasilevsky nicolevasilevsky requested a review from sbello August 3, 2022 21:13
@nicolevasilevsky
Copy link
Contributor Author

Lastly, @sbello thanks for following up on this issue on the call! 😸

@sbello
Copy link
Contributor

sbello commented Aug 8, 2022

  1. I think we should announce that this is the plan and give people some time to clean up usage. If we don't obsolete the existing absent term people will continue to use it.
  2. Absent some vs absent all absent some vs absent all #513

Copy link
Contributor

@sbello sbello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@nicolevasilevsky nicolevasilevsky merged commit c0de187 into master Aug 9, 2022
@nicolevasilevsky nicolevasilevsky deleted the issue-331 branch August 9, 2022 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants