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

Draft 1: Update confirmed subclass of evidence in Mondo #7801

Merged
merged 7 commits into from
Jul 30, 2024
Merged

Conversation

matentzn
Copy link
Member

This PR

  • Adds a workflow for updating confirmed subclass of evidence
  • Updates mondo-edit.obo accordingly

Issues of dispute

Against our previous agreement, the current version of the "confirmed" subclassOf axioms covers "Case 2 (SCR is direct in source, but indirect Mondo)". So, in other words, we include subclassOf axioms that are implied by Mondo (not asserted in the edit file!) and confirmed by an external source.

Example:

[Term]
id: MONDO:0000266
name: pulmonary aspergilloma
def: "A aspergillosis that involves the lung." [MONDO:patterns/location]
...
intersection_of: MONDO:0005657 ! aspergillosis
intersection_of: disease_has_location UBERON:0002048 ! lung

results in this:

[Term]
id: MONDO:0000266
name: pulmonary aspergilloma
def: "A aspergillosis that involves the lung." [MONDO:patterns/location]
...
is_a: MONDO:0005657 {source="DOID:0050153"} ! aspergillosis
intersection_of: MONDO:0005657 ! aspergillosis
intersection_of: disease_has_location UBERON:0002048 ! lung

https://docs.google.com/document/d/1H8fJKiKD-L1tfS-2LJu8t0_2YXJ1PQJ_7Zkoj7Vf7xA/edit#heading=h.9hixairfgxa1

Chris would probably be very against this, and I must say, I am also against it, but weakly. Indeed, I am like 55% against it, and 45% think this could be a good idea. The advantage is that we see all the evidence as Mondo curators - every positive subsumption. When we make classification decisions in the future, we can see them all in the edit file (not just the non-redundant ones). Note - all of these are implied, so they are all correct. Today.

The clear disadvantage is though that changing our logical definitions / patterns (equivalent class statements) will no longer be sufficient to change classification - if you really want to get rid of some subsumption moving forward, you must physically delete both the equivalence class axioms and the asserted subclass axiom. Here is a particularly bad scenario: We decide a pattern no longer works and change it, which leads to changes in classification. The old classification will not be removed, as the subclass of axioms have now been baked into the class hierarchy. I dont know though how much of a problem this is in reality.

I would suggest you

  1. Consult Chris on slack about his opinion on this. Its a big, PI-level, change.
  2. If he says "no", we need to change the source for the "confirmed subclass pipeline" to "mondo-edit". Its not a huge deal, but may require some thinking.
  3. If he says "yes", we still need to determine if there is a bug in this PR or not - I spot-checked 10 random examples and they are all correct (correct in the sense, mondo entails the subclass axiom).

This workflow implements our subclass syncining strategy:

1. First, we drop all axiom annotations related to the resources we sync with
2. We add them back
@sabrinatoro
Copy link
Collaborator

I don't mind the redundancy. In fact, I think it is helpful as it adds a level of confidence to the term classification (which is always good since Mondo integrates multiple sources).

We could consider removing this redundancy in the released version of the ontology (actually we might already do it), but I personally would keep it.

Copy link
Collaborator

@sabrinatoro sabrinatoro left a comment

Choose a reason for hiding this comment

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

I reviewed a few changes in the mondo-edit.obo file, and these seems as expected.
I found new source evidence on existing SC annotations, and new SC annotations+source.

@twhetzel, please take a look, especially for the changes in the subclass.yaml and mondo.Makefile files. Please merge if you approve. Thank you!

@twhetzel twhetzel assigned matentzn and unassigned twhetzel Jul 20, 2024
@matentzn matentzn assigned twhetzel and unassigned matentzn Jul 21, 2024
Copy link
Collaborator

@twhetzel twhetzel left a comment

Choose a reason for hiding this comment

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

Two suggested changes/commits

@twhetzel twhetzel assigned matentzn and unassigned twhetzel Jul 22, 2024
@twhetzel twhetzel self-requested a review July 22, 2024 06:16
@matentzn
Copy link
Member Author

thx @twhetzel, i will wait for your approval to merge!

Copy link
Collaborator

@twhetzel twhetzel left a comment

Choose a reason for hiding this comment

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

lgtm

@matentzn matentzn merged commit b2a1a8d into master Jul 30, 2024
1 check passed
@matentzn matentzn deleted the subclass-sync branch July 30, 2024 08:57
twhetzel added a commit that referenced this pull request Jul 31, 2024
* Create first draft of subclass sync

This workflow implements our subclass syncining strategy:

1. First, we drop all axiom annotations related to the resources we sync with
2. We add them back

* Update the subclassOf evidence in Mondo

* Add DOID, NCIT, OMIM and ORDO evidence to subclass axioms

* Update .github/workflows/subclass.yaml

Co-authored-by: Trish Whetzel <[email protected]>

* Update src/ontology/mondo.Makefile

Co-authored-by: Trish Whetzel <[email protected]>

---------

Co-authored-by: Trish Whetzel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants