-
Notifications
You must be signed in to change notification settings - Fork 8
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
Bypass punning in ontodoc. #532
Conversation
Also added a test to check that ontodoc actually runs with non-punned individuals and punned individuals
ontopy/ontodoc.py
Outdated
except TypeError: | ||
warnings.warn( |
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.
Very good warning message. Are we sure that there are not other TypeErrors what will be catched by this except statement?
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.
No, I am not sure that there are not others. But I think it is only punning that will be caught by this because we ask for an indivudal, and then we iterate of the concepts that the individual is an instance of. But I think this is a good point. If there is punning I know that is_instance_of returns 'property' and not a list of concepts, so I changed it to be a positive test for that. Better?
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 good. Added a question
…EMMOntoPy into flb/bypass_punning_in_ontodoc
Pushed with no verify as the pylint errors are due to pylint upgrade which finds errors in other parts of the core. This is addressed in another PR.
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.
Yes, this looks very good.
I have not tested it locally which I normally would do if I had my linux machine at hand, but I see you have a good test.
As far as I can see, PR #535 fixes the issue with failing tests. Please merge that into this branch before merging to master.
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.
Accepted given that the test passes
Note, only the ontodoc tests have been updated, so all other tool tests will fail.
This reverts commit a80a96e.
…_ontodoc Fix fixtures for Python3.7.
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #532 +/- ##
==========================================
+ Coverage 65.59% 65.84% +0.24%
==========================================
Files 16 16
Lines 3090 3095 +5
==========================================
+ Hits 2027 2038 +11
+ Misses 1063 1057 -6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Description:
Closes #506.
Ontodoc fails when punned individuals are included in the ontology.
After discussions with various people (see #507) we have agreed to not support punning for now.
This might be changed in the future if we see the need.
Tests have been added to see that ontodoc runs for ontologies with an individual, and that it runs with a warning
when the individual is a punned individual. Pytest is set to ignore that particualr warning so that this test
will continue to pass if pytest is set to fail on warnings as well in the future.
Type of change:
Checklist:
This checklist can be used as a help for the reviewer.
Comments: