-
Notifications
You must be signed in to change notification settings - Fork 14
Uncommented existing test_cli for "oaei-ordo-hp" #378
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
Conversation
|
@matentzn : ok, so I'm running : and the test.tsv begins Is the |
sssom/parsers.py
Outdated
| SUBJECT_SOURCE | ||
| ] = ( | ||
| e.firstChild.nodeValue | ||
| ) # CURIEfy this node value && If endswith(".extension"), replace it with _) |
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.
What is your plan here re curification if an URI prefix is not in the prefix map?
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 may have to either brute-force the CURIE or think if this info is secured via a curie_map in one of the yaml files?
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 would vote for leaving the URIs as is if the URI prefix is not known to the prefix map. We should produce a single warning at the end of the pipeline which lists the first 5 URIs that didn't have a proper CURIE, and log a warning "the extracted file is not valid SSSOM because some of the URIs could not be converted. Please provide appropriate URI prefixes and repeat the process".
Charlie prefers to not process the node at all if the URI prefix is unknown; this is an alternative, but only if we can overwrite it with a flag, and then we are back to the first point.
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.
That is the problem. The sssom definition for subject_source and object_source is EntityReference. Does this mean only CURIEs? uriOrCurie might be a better solution?
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.
Oh you don't want it to be a URI at all.
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 a URI is a curie, where the prefix is http and the URI expansion http so everwhere where there is an entity reference, you should be able to drop a URI.
Its fine to write a URI, just also write the warning I mention above.
matentzn
left a comment
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.
What is missing in this PR?
Just the curiefication |
matentzn
left a comment
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 great, two minor comments!
The test for parsing
oaei-ordo-hp.rdfwas commented. This PR just uncomments it and wires is properly intest_cli.pyto verify if it passes.