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

WIP: Remove string & object types to clean up the context #35

Merged
merged 16 commits into from
Jul 1, 2019

Conversation

natachaperez
Copy link

I use the same process as for boolean type e.g. I added the line: ctxt = re.sub(r',\s*\n\s*"@type": "http://www.w3.org/2001/XMLSchema#string"', '', ctxt) in create_nidmr_context.py. Most of the issues are cleaned up, we have now the following structure : "attribute description": "@id" : " <> expect for two of them : NIDM_0000157 and NIDM_0000159.

@natachaperez
Copy link
Author

natachaperez commented Jun 17, 2019

For the attributes "noiseFWHMInUnits" and "noiseFWHMInVoxels" in the SearchSpaceMaskMap class whose IDs are : NIDM_0000157 and NIDM_0000159, the method is slightly different. In Protegé, they match with spm files which are deprecated and still appear in the file nidm-results.owl. It seems that alter the reading of the nidm files. Hence, I choose to remove those lines corresponding to the spm identifiers : from line 952 to 963 and from line 979 to 991 in nidm-results.owl.
By running the file refresh.py and then opening the example spm-resullts.json, the context is now completely cleaned for string types.

@cmaumet
Copy link
Owner

cmaumet commented Jun 17, 2019

@natachaperez: thanks! It would be nice to better understand why this is happening though. Can you try and identify which line (or minimal subsets of lines) has to be removed from the owl file to obtain the behavior we want?

@natachaperez
Copy link
Author

natachaperez commented Jun 17, 2019

@cmaumet I tried line by line and we have to remove these following lines to obtain the behavior we want :

@cmaumet
Copy link
Owner

cmaumet commented Jun 21, 2019

@natachaperez: thanks a lot for those edits! For documentation purposes, can you add a comment explaining briefly what changes you introduced in the last set of commits and why?

@natachaperez
Copy link
Author

natachaperez commented Jun 21, 2019

So I introduce those following lines (in bold) in create_nidmr_context.py:
for s, o in sorted(owl.graph.subject_objects(SKOS['prefLabel'])):
json_key = str(o)
context['@context'][json_key] = OrderedDict()
if s in owl.ranges:
context['@context'][json_key]['@id'] = str(s)
context['@context'][json_key]['@type'] = next(iter(owl.ranges[s]))
else:
context['@context'][json_key] = str(s)
if owl.is_deprecated(s):
del context['@context'][json_key]

for json_key in context['@context']:
    if '_' in json_key:
       new_key = json_key.split('_')[1] 
       context['@context'][new_key]=context['@context'].pop(json_key)

These changes fix the problem encountered with the attributes "NoiseFWHMInUnits" and "FWHMInVoxels" without changing the owl file. Indeed, the issue was linked with the presence of the same json_key twice whereas a key must be unique in a dictionary. The idea was to change the json_key ( e.g. to delete the '_') after having completed the context. Then, we must ensure that none of deprecated labels remains in the context.

@natachaperez
Copy link
Author

natachaperez commented Jun 25, 2019

The changes above in create_nidmr_context enable to gather the lines previously added:
ctxt = re.sub(r',\s*\n\s*"@type": "http://www.w3.org/2001/XMLSchema#boolean"', '', ctxt)
ctxt = re.sub(r',\s*\n\s*"@type": "http://www.w3.org/2001/XMLSchema#string"', '', ctxt)
to clean up the context by keeping only the primitive type ( int, float, positiveInteger, Integer).
Moreover, we also figure out a way to fix the issues with object properties.

@cmaumet cmaumet changed the title WIP: Remove string type to clean up the context WIP: Remove string & object types to clean up the context Jun 25, 2019
@natachaperez
Copy link
Author

I added a check with the autopep8 command in order to be in line with the PEP8 convention for the python file create_nidmr_context and I remove the useless lines as comments.

Copy link
Owner

@cmaumet cmaumet left a comment

Choose a reason for hiding this comment

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

Hi @natachaperez!

Thanks for this pull request. I only have a few minor comments regarding remaining PEP8 formatting issue and creating one variable to make the code easier to read. Once this is updated, the pull request will be ready to merge.

RELPATH = os.path.dirname(os.path.abspath(__file__))
NIDMRESULTSPATH = os.path.dirname(RELPATH)
NIDMPATH = os.path.join(NIDMRESULTSPATH, os.pardir)
RELPATH = os.path.dirname(os.path.abspath(__file__))
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
RELPATH = os.path.dirname(os.path.abspath(__file__))
RELPATH = os.path.dirname(os.path.abspath(__file__))

Copy link
Owner

Choose a reason for hiding this comment

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

@natachaperez: this looks like a remaining PEP8 issue (there should be no space at end of line). Can you look for all those lines where a space was added and remove them? You have an overview of all the changes at: https://github.com/cmaumet/nidm/pull/35/files

NIDMRESULTSPATH = os.path.dirname(RELPATH)
NIDMPATH = os.path.join(NIDMRESULTSPATH, os.pardir)
RELPATH = os.path.dirname(os.path.abspath(__file__))
NIDMRESULTSPATH = os.path.dirname(RELPATH)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
NIDMRESULTSPATH = os.path.dirname(RELPATH)
NIDMRESULTSPATH = os.path.dirname(RELPATH)

NIDMPATH = os.path.join(NIDMRESULTSPATH, os.pardir)
RELPATH = os.path.dirname(os.path.abspath(__file__))
NIDMRESULTSPATH = os.path.dirname(RELPATH)
NIDMPATH = os.path.join(NIDMRESULTSPATH, os.pardir)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
NIDMPATH = os.path.join(NIDMRESULTSPATH, os.pardir)
NIDMPATH = os.path.join(NIDMRESULTSPATH, os.pardir)

if s in owl.ranges:
context['@context'][json_key]['@id'] = str(s)
context['@context'][json_key]['@type'] = next(iter(owl.ranges[s]))
if 'http://www.w3.org/2001/XMLSchema#int' in next(iter(owl.ranges[s])) or 'http://www.w3.org/2001/XMLSchema#double' in next(iter(owl.ranges[s])) or 'http://www.w3.org/2001/XMLSchema#integer' in next(iter(owl.ranges[s])) or 'http://www.w3.org/2001/XMLSchema#positiveInteger' in next(iter(owl.ranges[s])) :
Copy link
Owner

Choose a reason for hiding this comment

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

To make the code easier to read, let's use a variable ranges to store what's in next(iter(owl.ranges[s])).

Suggested change
if 'http://www.w3.org/2001/XMLSchema#int' in next(iter(owl.ranges[s])) or 'http://www.w3.org/2001/XMLSchema#double' in next(iter(owl.ranges[s])) or 'http://www.w3.org/2001/XMLSchema#integer' in next(iter(owl.ranges[s])) or 'http://www.w3.org/2001/XMLSchema#positiveInteger' in next(iter(owl.ranges[s])) :
ranges = next(iter(owl.ranges[s]))
if 'http://www.w3.org/2001/XMLSchema#int' in ranges or 'http://www.w3.org/2001/XMLSchema#double' in ranges or 'http://www.w3.org/2001/XMLSchema#integer' in ranges or 'http://www.w3.org/2001/XMLSchema#positiveInteger' in ranges:

context['@context'][json_key]['@type'] = next(iter(owl.ranges[s]))
if 'http://www.w3.org/2001/XMLSchema#int' in next(iter(owl.ranges[s])) or 'http://www.w3.org/2001/XMLSchema#double' in next(iter(owl.ranges[s])) or 'http://www.w3.org/2001/XMLSchema#integer' in next(iter(owl.ranges[s])) or 'http://www.w3.org/2001/XMLSchema#positiveInteger' in next(iter(owl.ranges[s])) :
context['@context'][json_key]['@id'] = str(s)
context['@context'][json_key]['@type'] = next(iter(owl.ranges[s]))
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
context['@context'][json_key]['@type'] = next(iter(owl.ranges[s]))
context['@context'][json_key]['@type'] = ranges

@cmaumet
Copy link
Owner

cmaumet commented Jul 1, 2019

@natachaperez: are you happy for me to merge now or you have remaining edits that you would like to commit?

@natachaperez
Copy link
Author

+1 for the merger @cmaumet

@cmaumet cmaumet merged commit 12d79df into cmaumet:jsonld_export Jul 1, 2019
@cmaumet cmaumet deleted the jsonld_export_string branch July 1, 2019 08:41
@cmaumet
Copy link
Owner

cmaumet commented Jul 1, 2019

(Now included in incf-nidash#479).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants