search

incf-nidash / nidm-specs

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

added draft definitions and updated Named Individuals #459

Closed khelm closed 6 years ago

khelm commented 6 years ago

Added draft definitions for all NIDM terms and clean-up Named Individuals so that the only NI's are specific assessment instruments.

Also----------------------------------------------------------------------------------------- -QIBO terms should not be Named Individuals

-Remove owl:sameAs connections; use owl:synonym instead

-To match past tense format of Prov, change nidm:hasImageContrastType to nidm:hadImageContrastType and nidm:hasImageUsageType to nidm:hadImageUsageType and the label for nidm:NIDM_0000010 to "had fMRI Design .

-Remove Series as a Named Individual

khelm commented 6 years ago

The Travis error is because there is nidm:Task and http://purl.org/nidash/bids/task. We need to discuss the placement of terms that are used both in BIDS and NIDM on our next call.

cmaumet commented 6 years ago

@khelm: just tried to rerunning this locally as we discussed and here is the error I get:

$ python scripts/refresh.py 
Traceback (most recent call last):
  File "scripts/refresh.py", line 39, in <module>
    main()
  File "scripts/refresh.py", line 34, in main
    create_expe_specification.main()
  File "<ANON_PATH>/nidm-specs/scripts/../nidm/nidm-experiment/scripts/create_expe_specification.py", line 113, in main
    prefix=str(NIDM))
  File "<ANON_PATH>/nidm-specs/scripts/owl_to_webpage.py", line 20, in __init__
    self.owl = OwlReader(owl_file, import_files)
  File "<ANON_PATH>/anaconda2/lib/python2.7/site-packages/nidmresults-1.2.1.dev1-py2.7.egg/nidmresults/owl/owl_reader.py", line 53, in __init__
    (str(key), value) for (value, key) in labels
  File "<ANON_PATH>/anaconda2/lib/python2.7/collections.py", line 69, in __init__
    self.__update(*args, **kwds)
  File "<ANON_PATH>/anaconda2/lib/python2.7/_abcoll.py", line 571, in update
    for key, value in other:
  File "/<ANON_PATH>/anaconda2/lib/python2.7/site-packages/nidmresults-1.2.1.dev1-py2.7.egg/nidmresults/owl/owl_reader.py", line 54, in <genexpr>
    if not self.is_deprecated(value))
  File "<ANON_PATH>/anaconda2/lib/python2.7/site-packages/rdflib/term.py", line 1256, in __str__
    return self.encode()
UnicodeEncodeError: 'ascii' codec can't encode character u'\xf6' in position 8: ordinal not in range(128)

Just a guess but do you think there could be a non-ascii character in nidm-experiment.owl?

khelm commented 6 years ago

Error Message:

python scripts/refresh.py
Traceback (most recent call last):
  File "scripts/refresh.py", line 39, in <module>
    main()
  File "scripts/refresh.py", line 34, in main
    create_expe_specification.main()
  File "/home/karl/Work/INCF/nidm/nidm/scripts/../nidm/nidm-experiment/scripts/create_expe_specification.py", line 113, in main
    prefix=str(NIDM))
  File "/home/karl/Work/INCF/nidm/nidm/scripts/owl_to_webpage.py", line 20, in __init__
    self.owl = OwlReader(owl_file, import_files)
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/nidmresults/owl/owl_reader.py", line 39, in __init__
    self.graph = self.get_graph()
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/nidmresults/owl/owl_reader.py", line 386, in get_graph
    import_graph.parse(import_file, format='turtle')
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/graph.py", line 1039, in parse
    parser.parse(source, self, **args)
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 1870, in parse
    p.loadStream(source.getByteStream())
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 434, in loadStream
    return self.loadBuf(stream.read())    # Not ideal
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 440, in loadBuf
    self.feed(buf)
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 466, in feed
    i = self.directiveOrStatement(s, j)
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 487, in directiveOrStatement
    j = self.statement(argstr, i)
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 725, in statement
    j = self.property_list(argstr, i, r[0])
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 1086, in property_list
    i = self.objectList(argstr, j, objs)
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 1134, in objectList
    i = self.object(argstr, i, res)
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 1406, in object
    j = self.subject(argstr, i, res)
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 733, in subject
    return self.item(argstr, i, res)
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 829, in item
    return self.path(argstr, i, res)
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 837, in path
    j = self.nodeOrLiteral(argstr, i, res)
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 1431, in nodeOrLiteral
    j = self.node(argstr, i, res)
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 1049, in node
    j = self.uri_ref2(argstr, i, res)
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 1186, in uri_ref2
    "Prefix \"%s:\" not bound" % (pfx))
  File "/home/karl/Work/packages/anaconda2/lib/python2.7/site-packages/rdflib/plugins/parsers/notation3.py", line 1615, in BadSyntax
    raise BadSyntax(self._thisDoc, self.lines, argstr, i, msg)
rdflib.plugins.parsers.notation3.BadSyntax: at line 35 of <>:
Bad syntax (Prefix "prov:" not bound) at ^ in:
"...## http://purl.org/nidash/bids/task

dc:identifier rdf:type ^prov:entity ;

               obo:IAO_0000115 "An unambiguou..."
cmaumet commented 6 years ago

@khelm: I just checked and found that replacing "Fagerström" with "Fagerstrom" solves the bug above (although there are warnings)

cmaumet commented 6 years ago

Would you like for me to push that update in your branch? To check if that helps on your machine as well?

cmaumet commented 6 years ago

That's strange we have a different error, can you send the output of git status and git log -2?

khelm commented 6 years ago

Yeah, I saw the umlaut and was wondering if it would cause an issue. I'll fix on my end and rerun.

khelm commented 6 years ago

I got the same error after removing the umlaut o. Will see what Travis says here.

cmaumet commented 6 years ago

@khelm: can you try to run refresh.py again? The error in Travis is about the HTML file being outdated.

khelm commented 6 years ago

The problem is that somehow refresh.py is finding a file called bids.ttl in my imports directory. I'm not using it, but the code finds it and is unhappy. I'll delete it and rerun.

cmaumet commented 6 years ago

Okay, I understand now why we have the Travis error! Can you copy what is returned by both:

We need to check whether there is anything on your machine that makes the code different from what we have on GitHub.

cmaumet commented 6 years ago

Click here for a preview of the NIDM-Experiment spec (at commit 77f42f5).

khelm commented 6 years ago

I'm cleaning up the imports folder and getting rid of anything that is currently unused. I've been running refresh.py and then create_expe_specs.py as requested by the refresh.py error message, but when I run the latter I get the old No Prov type for class: errors.

khelm commented 6 years ago

@cmaumet - something isn't handling the Named Individuals correctly in nidm-experiment.owl (which are all of the assessment instrument entries). I noticed that Protege breaks them up into a Named Individual section and an Annotation section, which is not the case in nidm-results. Could this be an issue?

khelm commented 6 years ago

@cmaumet : there is something going on with the create_expe_specifications.py file that is causing the errors and warnings having to do with the subcomponent definitions. The terms in the Project and Acquisition subcomponents don't return a warning with refresh.py, but all of the other subcomponents do. UserWarning: No PROV type for class: nidm:CurrentClamp In addition, the subcomponent for Assessments returns the same warning but also an error saying that the assessment instrument terms "does not exist", which arises from owl_reader. I don't see anything different in how the subcomponents are defined, but can you please take a look. As above, I think the "does not exist" errors have to do with the terms being Named Individuals with split up entries. How does your code deal with Named Individuals?

cmaumet commented 6 years ago

@khelm: it is difficult for me to understand what is going on because I cannot reproduce the error on my machine. When I run python script/refresh.py, I do get warnings but no error and nidm-experiment_dev.html is updated. Let's first check why what we get is different?

Can you send the output of: git status, git log -2 and nidmresults --version?

Here is what I get:

$ git status 
On branch khelm/add-term-defs
Your branch is up to date with 'remotes/khelm/add-term-defs'.

nothing to commit, working tree clean

$ git log -2
commit 827000b1284145ceab567cbe16047332e436a5de (HEAD -> khelm/add-term-defs, khelm/add-term-defs)
Author: Karl Helmer <helmer@nmr.mgh.harvard.edu>
Date:   Tue Apr 10 15:41:16 2018 -0400

    reorder create_expe_specifications.py

commit 63285ef20eb015c99e327153f3899a79fdc7058e
Author: Karl Helmer <helmer@nmr.mgh.harvard.edu>
Date:   Mon Apr 9 20:55:57 2018 -0400

    modify terms in create_expe_specs

$ nidmresults --version
1.2.1.dev1

Thanks!

khelm commented 6 years ago

@cmaumet - here is the information you requested. Note that I've just made further changes to the files noted below, but they don't change the warnings:

$ git status
On branch add-term-defs
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

    modified:   ../../../doc/content/specs/nidm-experiment_dev.html
    modified:   create_expe_specification.py

no changes added to commit (use "git add" and/or "git commit -a")
$ git log -3
commit 827000b1284145ceab567cbe16047332e436a5de
Author: Karl Helmer <helmer@nmr.mgh.harvard.edu>
Date:   Tue Apr 10 15:41:16 2018 -0400

    reorder create_expe_specifications.py

commit 63285ef20eb015c99e327153f3899a79fdc7058e
Author: Karl Helmer <helmer@nmr.mgh.harvard.edu>
Date:   Mon Apr 9 20:55:57 2018 -0400

    modify terms in create_expe_specs

commit 5a3e1033fa9af8f7554f71fb34b495dea6c8f07d
Author: Karl Helmer <helmer@nmr.mgh.harvard.edu>
Date:   Mon Apr 9 12:24:14 2018 -0400

    rm unused cogpo import

Version:

$ nidmresults --version
1.2.1.dev0

The errors go away when I remove the Named Individuals from the create_expe_specifications file. The warnings are all there, but note that it throws a warning not for all of the terms in the create_expe_specifications.py file; the terms in the first two subcomponents give no warning.

cmaumet commented 6 years ago

Thanks for those outputs! I know it might sound cumbersome but that's the only way we can make sure we are in sync, which is so important to fix those issues.

If I understand correctly, you do not have errors anymore (only warnings)?

If that's correct, I would suggest that you: 1) commit new version of "create_expe_specification.py" 2) rerun python refresh.py and commit all changes 3) push.

If that goes well, we can then focus on the warnings and on displaying the named individuals. Does this sound okay?

khelm commented 6 years ago

No errors, because I removed the Named Individuals from the create_expe_specification.py file - they were giving the "no class" errors, which makes sense because they are - not classes. The other changes I made to the latest untracked files were just formatting, so I could make sure that all of the subcomponents had the same formatting. I've also updated my version of nidmresults in the meantime.

khelm commented 6 years ago

I've also rerun: nidm/nidm-results/scripts/create_term_examples.py nidm/nidm-results/scripts/create_results_specification.py nidm/nidm-results/scripts/create_expe_specification.py and no files were modified as a result.

cmaumet commented 6 years ago

Thank you for pushing this new version, we are making progress!

From what I see in the NIDM-Results spec, named individuals will automatically be displayed in the HTML specification if the class that corresponds to the type of those individuals (and not the individual themselves) is displayed. For example:

Now, the new errors you have (e.g. FSL_Software.txt is not up to date with templates. Please use nidm/nidm-results/scripts/create_term_examples.py.) are similar to what we had in the past when working with an outdated version of the nidmresults library. Sometimes it gets a bit confusing when there are multiple version of a library installed. I would recommend to:

  1. Uninstall any version of nidmresults by running the following command multiple times (until you get a message saying nidmresults not installed): 
    pip uninstall nidmresults
    1. Try again pip install git+git://github.com/incf-nidash/nidmresults.git to reinstall the latest version.
    2. Check the version with nidmresults --version.
    3. Rerun python scripts/refresh.py and see if that makes any change.
khelm commented 6 years ago

OK, with nidmresults 1.2.1 dev1 there are no warnings. Thanks!

cmaumet commented 6 years ago

Excellent!!

Have you used rawgit before? If you would like to include a link to a preview of the updated spec, you can use https://rawgit.com/ and paste in the link to your updated nidm-experiment_dev.html file, i.e. at https://github.com/khelm/nidm/blob/add-term-defs/doc/content/specs/nidm-experiment_dev.html the link you get by clicking on "raw" --> https://raw.githubusercontent.com/khelm/nidm/add-term-defs/doc/content/specs/nidm-experiment_dev.html

Note: in future pull requests, we could use the last link again, by updating "add-term-defs" with the name of the corresponding branch.

khelm commented 6 years ago

So, ok, to merge? No, I haven't used it and was wondering how you got the preview you pointed me to earlier. I'll take a look, this looks very useful for the future. Thanks!

cmaumet commented 6 years ago

Okay! Preview is here: NIDM-experiment specification.

What is your plan to discuss the proposed definitions with the group? We could leave this PR open while this process is happening and include any suggestion here. Another solution is to mark the definitions as 'uncurated' (or something similar) and open issues to discuss specific terms.

In any cases, it might be worth flagging this PR to the group (maybe by email?) before we merge and see if there is feedback.

I feel like we should try to make the guidelines clearer on how we review and merge PRs and how to propose new terms and edits. If I get started, would be okay to help me draft contributing guidelines to make this process more transparent?

khelm commented 6 years ago

I would rather merge and close the PR as all of the terms are marked as "has curation status: requires discussion". I liked the process that we used for Results in which we had a record of the discussion for each term or question on the structure. I'd like to get a page such as you have for Results in which you can see the status of each terms and we can just go down the list and open issues for discussion.

I'd be happy to help with the drafting of a process for the review and merging of PRs. I think that's a good idea.

cmaumet commented 6 years ago

That makes sense! Thanks! +1 to merge.