incf-nidash / nidm-specs

Neuroimaging Data Model (NIDM): describing neuroimaging data and provenance
nidm.nidash.org
Other
33 stars 30 forks source link

Fix import files for NIDM-Experiment #458

Closed khelm closed 6 years ago

khelm commented 6 years ago

Updated the @base and ontology declarations in the individual import files to be those declared already in the catalog-v001.xml file for NIDM-Experiment. Also reordered some of the @prefix declarations so that the first prefix is always that of the ontology for that particular import file. Renamed qibo_import.owl to qibo_import.ttl to be consistent with the naming convention for other import files.

cmaumet commented 6 years ago

Hi @khelm! Thanks for this PR, great to see this (this PR follows discussions to try and fix https://github.com/incf-nidash/nidm-specs/issues/453).

I think using the latest (dev) version of the nidmresults library on your machine should help with the errors above:

pip install git+git://github.com/incf-nidash/nidmresults.git

Can you try this and then rerun the refresh script?

khelm commented 6 years ago

Hi @cmaumet - I tried to update my nidm as you suggested, but it looks as though it's already up-to-date and I'm still getting the same warnings as below, when re-running refresh.py:

$ python scripts/refresh.py
/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/nidmresults/owl/owl_reader.py:234: UserWarning: No PROV type for class: nidm:Model
  self.graph.qname(class_name))
/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/nidmresults/owl/owl_reader.py:234: UserWarning: No PROV type for class: nidm:OrganismType
  self.graph.qname(class_name))
/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/nidmresults/owl/owl_reader.py:234: UserWarning: No PROV type for class: nidm:AcquisitionMethod
  self.graph.qname(class_name)) 

etc...

When I look at the Travis errors, I see the recommendation to run nidm/nidm-results/scripts/create_term_examples.py. Doing this doesn't create any new files that show up in git status. The error after the warnings above also recommends that I run: python nidm/nidm-results/scripts/create_results_specification.py dev, but again this doesn't result in any file changes that git status sees.

In addition, the Travis error log also shows that running python test/test_specifications.py results in warnings for what looks like all of the NIDM-Results terms, says that each term in not a valid URI.

cmaumet commented 6 years ago

@khelm: sorry for getting back on this but the Travis errors where scicrunch prefix being is src instead of scr makes me think that nidmresults might be outdated... Can you try again pip install git+git://github.com/incf-nidash/nidmresults.git and then check the version with nidmresults --version. I've just added a dev suffix so that we can identify the dev version more easily. On my machine I get:

$ nidmresults --version
1.2.1.dev0
khelm commented 6 years ago

Updating nidm to 1.2.1.dev0 fixes the Travis errors. Thanks for merging this @cmaumet !

khelm commented 6 years ago

The IAO reference must be new - thanks for finding that! I've updated the nidm-experiment.owl file to use qibo_import.owl as you suggest. I also notice that some of the imported ontologies are also referred to above in the prefix list. I don't think they should be there - should they?

Yes, I agree that we should keep this open until the warnings are resolved.

cmaumet commented 6 years ago

I have tested on my machine and can confirm that the imports now work fine: screen shot 2018-03-06 at 09 33 56

I am thinking there must be a bug in the nidmresults library that raises the warnings.

khelm commented 6 years ago

I'm going to remove the @prefix statements for the ontologies that are imported in the nidm-experiment.owl file and see what happens. It seems to me that this could be a source of confusion.

khelm commented 6 years ago

@cmaumet - there are also a number of ontologies in the header that are never used in the nidm-experiment.owl file. They are: ro, bfo, xml, xsp, daml, meta, owl2, snap, span, swrl, swrlb, owl2xml, oboInOwl. The ontologies that are both imported and defined using an @prefix statement are: nls, sio, onli, qibo. Are you ok with removing them in the header?

cmaumet commented 6 years ago

Yes, removing them sounds good!

khelm commented 6 years ago

@cmaumet - I've been looking into the warnings that I get for all nidm namespace terms when I run refresh.py. The warning comes from line 238 in owl_reader.py that is:

if not prov_type_found:
                        warnings.warn('No PROV type for class: ' +
                                      self.graph.qname(class_name))
                        class_names.setdefault(None, list()).append(class_name)

but this is determined by a call above to

prov_type = self.get_prov_class(class_name)

on line 221.

Where this seems to break for the NIDM namespace terms is:

           if not prov_class:
                if recursive > 0:
                    parent_classes_super = parent_classes
                    for parent_class_super in parent_classes_super:
                        if not isinstance(parent_class_super, term.BNode):
                            prov_class = self.get_prov_class(
                                parent_class_super, recursive=recursive-1)
                            if prov_class is not None:
                                break

Here recursive is originally set to 3 so when the code looks for the prov class is it then going back only 2 nodes in the code above?

cmaumet commented 6 years ago

@khelm: sorry for being so slow... The tests now pass without the "No PROV type" warnings!

You were right, it was necessary to increase the recursion level in nidmresults (now set to 6, cf. https://github.com/incf-nidash/nidmresults/pull/44).

The few last commits also:

khelm commented 6 years ago

Thanks very much for taking the time to look into this @cmaumet! Ok to merge?

cmaumet commented 6 years ago

Preview of the updated spec: nidm-e

cmaumet commented 6 years ago

Thanks for your patience! I am happy for you to merge!

khelm commented 6 years ago

Thanks for pointing to the revised spec doc. One thing I noticed is that not all imported terms are in this doc. There are some, as you pointed out sio: and neuro: terms, but not all of the imported namespaces are represented. For example, the terms for qibo: are not there. Is the specs doc only supposed to contain nidm: terms or also all other imported terms?

cmaumet commented 6 years ago

I think that the HTML spec will include:

Therefore the answer of your question depends on where the qibo terms are located and how they are linked to other terms...

Note the script to create the spec is not set in stone and we could update it if we think we need new features.

cmaumet commented 6 years ago

Thinking about it, I think I had an error message in Protege about the qibo import, maybe that's why we can't see the qibo terms?

khelm commented 6 years ago

I fixed all of the imports in an earlier commit 61b959b and an even earlier one 06403be and they should all have consistent prefixes. Do you also need to bind the prefixes for the other imports as you did for sio: and neuro:?

cmaumet commented 6 years ago

@khelm: I don't think the issue is with the binding of the prefixes (if they are not binded the prefixes display as 'default' or 'ns' in the spec but they are still there)... One thing I could recommend is to try and refer to a QIBO term directly in create_expe_specification.py and see if it displays in the spec. @khelm: do you think you could try this up?

khelm commented 6 years ago

The reason I didn't try that is that some sio: term and neuro: terms showed up in the specs webpage, but are not in the create_expe_specification.py file. But also not all of the sio: terms didn't show up. Any idea why?

cmaumet commented 6 years ago

Is it possible that the displayed sio terms all have direct relations with to the terms explicitly included in create_expe_specification.py?

e.g. sio:'clinical trial' is a child of nidm:Project:

screen shot 2018-03-19 at 16 42 57

khelm commented 6 years ago

@cmaumet

A couple of questions:

1) Is the Introduction list on the specs page supposed to be only NIDM terms or a full representation of the tree (NIDM+imported terms)? Mostly the NIDM terrms are at the leaf ends of the tree and there are only a few cases in which non-NIDM terms are child terms of NIDM terms. Those few cases show up in the Introduction tree at the top of the NIDM-Exp specs page. Parent terms of NIDM terms don't show up in the Intro tree. So, in:

prov:term
     sio:term1
         nidm:term2
               sio:term3

both term2 and term3 will show up in the tree at the top of the specs page, but not term or term1.

2) If it's supposed to be the full graph, then I'll add all of the other import terms to create_expe_specification.py and find a larger set of categories to place the terms under. The issue is that in most cases the imports form the "intermediate-level" ontologies (assuming we call prov the top-level ontology) and the NIDM terms are almost always the leaf terms. This means that the organizing categories that are currently in use will probably not work for the imported terms.

Whatever we decide, I'm going to merge this as this has moved on to being a new issue.

cmaumet commented 6 years ago

@khelm: I think this is very much up to you and how you think the spec page should be organised to be the most understandable. One thing that we could do is to also add information about parent terms (in addition to children), e.g. we could have something like:

 nidm:'Project' is a prov:Activity and <PARENT_TERM> and has the following child: sio:'clinical trial'.

How does this sound? Also, let me know if I should continue this discussion in another issue? Thank you!