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

included / formerly refactor #167

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Conversation

joeflack4
Copy link
Contributor

@joeflack4 joeflack4 commented Nov 20, 2024

Changes

OMIM included & formerly refactor

  • Update: Made OMIM_INCLUDED & OMIM_FORMERLY synonymType
  • Update: OMIM_INCUDED is now a type on relatedSynonym rather than its own declaration. This result in addition of new synonyms.
  • Update: Renamed these to UPPER_SNAKE_CASE
  • Add: Missing property declarations
  • Deleted: rdfs:comment related to 'included'
  • Refactor: Added abstraction: add_included_synonym()
  • Add: *.obo to .gitignore. This file type will only be used ad hoc to examine outputs in .obo form.

Related:

- Rename to be in line with what is in Mondo: includedEntryInOMIM
Copy link
Member

@matentzn matentzn left a comment

Choose a reason for hiding this comment

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

Checked it in mondo, looks correct

omim2obo/main.py Outdated
@@ -164,7 +164,7 @@ def omim2obo(use_cache: bool = False):
# - Non-OMIM triples
graph.add((URIRef('http://purl.obolibrary.org/obo/mondo/omim.owl'), RDF.type, OWL.Ontology))
graph.add((URIRef(oboInOwl.hasSynonymType), RDF.type, OWL.AnnotationProperty))
graph.add((URIRef(MONDONS.omim_included), RDF.type, OWL.AnnotationProperty))
graph.add((URIRef(MONDONS.includedEntryInOMIM), RDF.type, OWL.AnnotationProperty))
Copy link
Contributor Author

@joeflack4 joeflack4 Dec 16, 2024

Choose a reason for hiding this comment

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

Rename

Discussed w/ Nico today. These are not the same.We need to keep both.

  • @joeflack4 Rename omim_included to omimIncluded or OMIM_INCLUDED (see: comment)
    • Do same for 'formerly'
  • @joeflack4 Don't remove includedEntryInOMIM

@matentzn
Copy link
Member

Look how clingen_label is formatted I think it's all upper case with underscore, so OMIM_INCLUDED should be as well (always check other examples of the same thing you are adding to determine correct casing)

omim2obo/main.py Outdated
@@ -164,7 +164,7 @@ def omim2obo(use_cache: bool = False):
# - Non-OMIM triples
graph.add((URIRef('http://purl.obolibrary.org/obo/mondo/omim.owl'), RDF.type, OWL.Ontology))
graph.add((URIRef(oboInOwl.hasSynonymType), RDF.type, OWL.AnnotationProperty))
graph.add((URIRef(MONDONS.omim_included), RDF.type, OWL.AnnotationProperty))
graph.add((URIRef(MONDONS.includedEntryInOMIM), RDF.type, OWL.AnnotationProperty))
Copy link
Contributor Author

@joeflack4 joeflack4 Dec 17, 2024

Choose a reason for hiding this comment

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

Snake or camel case

Nico wrote:

Look how clingen_label is formatted I think it's all upper case with underscore, so OMIM_INCLUDED should be as well (always check other examples of the same thing you are adding to determine correct casing)

Originally we were doing snake case, but then we 3 met a few weeks ago and coming out of that meeting, determined this and omimFormerly should be camel case, but I don't remember why.

Edit:

    <owl:AnnotationProperty rdf:about="http://purl.obolibrary.org/obo/mondo#CLINGEN_LABEL">
        <rdfs:subPropertyOf rdf:resource="http://www.geneontology.org/formats/oboInOwl#SynonymTypeProperty"/>
    </owl:AnnotationProperty>

@twhetzel Can make the judgement on this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on this, I think UPPER_SNAKE_CASE is best.

Base automatically changed from develop to main December 19, 2024 00:33
@joeflack4 joeflack4 changed the title MONDO:omim_included rename MONDO:omim_included refactor Jan 3, 2025
@joeflack4 joeflack4 changed the title MONDO:omim_included refactor included / formerly refactor Jan 3, 2025
@joeflack4 joeflack4 force-pushed the omim-included-rename branch 3 times, most recently from 1ab69db to 4522ba5 Compare January 4, 2025 23:07
@joeflack4 joeflack4 marked this pull request as ready for review January 4, 2025 23:08
@joeflack4 joeflack4 requested review from matentzn and twhetzel January 4, 2025 23:08
@joeflack4 joeflack4 force-pushed the omim-included-rename branch from 4522ba5 to 0bd7c5a Compare January 4, 2025 23:24
@@ -141,6 +142,19 @@ def add_subclassof_restriction_with_evidence_and_source(
add_axiom_annotations(graph, on, RDFS['subClassOf'], b, annotation_pred_vals)


def add_included_synonym(graph: Graph, omim_uri: URIRef, synonym: str, is_symbol=False, is_formerly=False):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Several important changes

Trying to to open threads often. But I felt these were really important changes I should mention here.

Nico though it made sense for OMIM_INCLUDED and OMIM_FORMERLY to be synonym types, and Trish and I have started to discuss at meetings but haven't had time to fully dive in. If these changes are too much, I can undo.

  1. These are synonym types now
  2. The spelling/caps are now different
  3. If they are synonym types, that means I need to be adding synonyms for OMIM_INCLUDED, which we weren't doing before. This is kind of a major change. I chose hasRelatedSynonym.
  4. owl:deprecated was being added for OMIM_FORMERLY now I think does not make sense to add to hasRelatedSynonym, so I removed it. We could also use MONDO:DEPRECATED, but I think that maybe OIMIM_FORMERLY stands on its own for explanation and might not need any additional annotations.

- Update: Made OMIM_INCLUDED & OMIM_FORMERLY synonymType
- Update: Renamed these to UPPER_SNAKE_CASE
- Update: OMIM_INCUDED is now a type on relatedSynonym rather than its own declaration. This result in addition of new synonyms.
- Update: Retained includedEntryInOMIM as annotation property on synonyms
- Add: Missing property declarations
- Deleted: rdfs:comment related to 'included'
- Refactor: Added abstraction: add_included_synonym()
@joeflack4 joeflack4 force-pushed the omim-included-rename branch from 0bd7c5a to e806fe2 Compare January 4, 2025 23:55
- Delete: includedEntryInOMIM stuff just added. I had thought we had previously had this in the codebase, but we had not. This is an xref source value, but the way I was adding it in the last commit anyway was a bit confused; I had been adding it as a synonym source value.
@joeflack4 joeflack4 changed the base branch from main to develop January 4, 2025 23:57
Copy link
Member

@matentzn matentzn left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants