-
Notifications
You must be signed in to change notification settings - Fork 29
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
Fixes #3288 Add blood vasculature design pattern and 55 blood terms from VCCF #3289
Conversation
…rom VCCF Fixes #3288 Add blood vasculature design pattern and 55 blood terms from VCCF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,56 @@ | |||
defined_class label parent part_of_vccf definition xref_vccf synonym synonym_xrefs connecting_branch_of tributary_of vessel_supplies_blood_to vessel_drains_blood_from creator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
label
will not be used, check docs. I think it must be defined_class_name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as definition
- are they too irregular to automatically generate them with dosdp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The big advantage of DOSDPs over ROBOT is text generation. This can use optionals too. If really need to add labels and defs manually, then maybe ROBOT is more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I prefer to not automatically generate them, since they have different structures. I can use ROBOT then, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the main advantage of DOSDPs is that they are a specification here, independent of whether you want to use the spec to generate or validate. We are desperately in need of specifications of all the patterns in Uberon
hasDbXref: "oboInOwl:hasDbXref" | ||
|
||
vars: | ||
parent: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not typing these? Why "" instead of, say, "anatomical entity"?
parent
is not a name for a slot in a dosdp pattern. Sounds again like a ROBOT template to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can call your slots whatever you want, but it is good practice to specify a range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was adapting the design pattern that we have for the VCCF ontology, but yes, I should have added a range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we completely sure we want a single DP covering all blood vessels? I prefer separating artery veins and direction-agnostic classes capillaries etc into distinct DPs (with reuse of slots).
The TSV and the YAML don't seem to match. What is xref_vccf
?
Thank you, I have split the pattern in two patterns, one for veins/venules and one for arteries/arterioles. I will create a DOSDP for capillaries in the future.
Apologies, the column was supposed to be relabeled and it will be fix in the next commit. |
Changes in the DOSDP: - Split in two DOSDP for venous and arterial blood vessels - Add proper description - Add range to variables - Logical axioms that might have more than one variable, use list_vars and multi_clause Changes in the table: - Fix column labels - Due a problem on the range of vessel supplies/drains blood to/from, all anatomical spaces or cavities had to be substituted
Changes in the DOSDPs:
Changes in the tables:
|
Once the object properties are fixed, needs to be reversed
…type/uberon into 3288_vccf_DOSDP_vessels
#gogoeditdiff |
Here's a diff of how these changes impact the classified ontology (on -base file):Ontology comparisonLeft
Right
Ontology importsOntology annotationsRO_0020101
|
Here's a diff of your edit file (unreasoned)Ontologies are identical |
- Change column name part_of for location - Add FMA_xref - All radiopedia URL are substituted by doi - Add more specific ranges
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM to me, but I didnt do a deep dive.
@aleixpuigb I absolutely love it you took the time to carefully craft these patterns. This is super essential for Uberon moving forward!
Thanks a lot!
I will leave the deep dive review to @rays22 and @anitacaron
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine for the most part, but there are some typos and yaml
syntax issues. The logical axioms look also fine for now, but I am not sure if all the anatomy experts would agree with the resulting taxonomy.
Co-authored-by: Ray Stefancsik <ray@ebi.ac.uk>
This commit fixes: - Unnecessary indentation - Add new empty line at the end of the yaml files - Change the range of date to xsd:dateTime - Fix typo in atrioventricular nodal artery
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the pattern templates look fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, I approve it.
Fixes #3288 Add blood vasculature design pattern and 55 blood terms from VCCF