-
Notifications
You must be signed in to change notification settings - Fork 89
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
owlgen: no class needed for terms defined on other vocabularies #1287
base: main
Are you sure you want to change the base?
Conversation
Hi @Silvanoc - apologies we let this one slip. Generally things that are not passing github actions fall behind in our PR review priorities, but I would be very keen to get your changes in, let us know if you are able to do another pass to get tests to pass, or if you want one of us to take this over |
c97ad1d
to
c0e43de
Compare
No, problem. It's understandable that PRs not passing tests fell down in your priority list. But I couldn't find a better way to provide some non-ready code as a discussion basis. That's why added the initial prominent note 🙂 I've tried to document in the description and commit messages what they're trying to accomplish. I lack deep knowledge about the LinkML internals to be sure that I'm fixing it "the right way". Therefore I'd appreciate if you could have a look at the description, commit messages and drafted patch and consider taking over. |
What wonders me is that changes in the OWL generator breaks test in multiple, apparently unrelated, places (starting with Am I really breaking all those tests? Or simply triggering something else (like caches being erased) that is bringing the tests to new "unexplored" paths that make them fail? |
I've decided to split this PR in two, since one of the commits is fixing what I'm sure is a bug. That goes to PR #1375. I'd expect the pipelines to succeed on that PR. The other commit is fixing what I consider a bug, but it's discutible. I'll keep this single commit in this PR. This way we have more focussed discussions. |
c0e43de
to
789a642
Compare
789a642
to
e985a19
Compare
It is not possible to inherit from terms from external vocabularies (for example, you cannot write "is_a: schema:Person"), since those terms are not available in the schema. One way to make them available in the schema is explicitly declaring them like this (for the above example with "schema:Person": Person: class_uri: schema:Person but it creates an unexpected class declaration in the OWL ontology saying that "schema:Person is an exactMatch of schema:Person". This patch gets rid of all those superfluous declarations. Signed-off-by: Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>
e985a19
to
241ddc5
Compare
Important NOTE: this MR is a PoC or demo not ready for merging for the time being. I'm lack the expertise to asses if it's breaking something else.
This MR tries to address the issues mentioned in #1281.
If a class or slot are to be inherited from terms defined in external vocabularies (for example, https://rds.posccaesar.org/ontology/lis14/rdl/ in my case), a declaration is needed in the schema. That declaration provokes unexpected "exactMatch" entries instead of the expected "subClassOf"/"subPropertyOf".
Example
Following schema
Results in an OWL file with following entries:
Notice that
FunctionalObject
gets a wrong URI, since it should berdl:FunctionalObject
. That's what the first commit tries to fix.<https://example.org/my-schema/mySubslot>
is already being defined as ardfs:subPropertyOf rdl:hasFeature
, so no need to have therdl:hasFeature skos:exactMatch rdl:hasFeature
at the bottom. That's what the second commit tries to fix.With the patch the output is what I'd expect:
without neither
rdl:Functional skos:exactMatch rdl:Functional
norrdl:hasFeature skos:exactMatch rdl:hasFeature
.