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

Correct saving squashed ontology #719

Merged
merged 11 commits into from
Feb 29, 2024
Merged

Conversation

jesper-friis
Copy link
Collaborator

@jesper-friis jesper-friis commented Feb 25, 2024

Description

Added --iri and --base-iri options to ontoconvert allowing to correctly saving a squashed ontology.

This is both a fix in EMMOntoPy and a workaround for the bug in Owlready2 that replaces the final hash (#) of the base_iri to a slash (/).

Test it with:

cd EMMO
git fetch origin 1.0.0-beta7:1.0.0-beta7
git co 1.0.0-beta7
ontoconvert -wsa -b "https://w3id.org/emmo#" -I "https://w3id.org/emmo" emmo.ttl emmo-squashed.ttl

Note: Had to add a lot of

# pylint: disable=not-an-iterable

because pylint doesn't understand that rdflib.triples() returns an iterable... This is not the responsibility of EMMOntoPy to fix.

Note 2:
Got rid of badly designed tests/tools/conftest.py and replaced it with a generally usable testutils module, which can be reused both from within tests and in conftest files if someone needs the latter.

Great code simplification and reduction.

Note 3:
Consider to add real tests with asserts to the tool tests, like what is done for ontoconvert in this PR.

Type of change

  • Bug fix.
  • New feature.
  • Documentation update.
  • Test update.

Checklist

This checklist can be used as a help for the reviewer.

  • Is the code easy to read and understand?
  • Are comments for humans to read, not computers to disregard?
  • Does a new feature has an accompanying new test (in the CI or unit testing schemes)?
  • Has the documentation been updated as necessary?
  • Does this close the issue?
  • Is the change limited to the issue?
  • Are errors handled for all outcomes?
  • Does the new feature provide new restrictions on dependencies, and if so is this documented?

Comments

Copy link

codecov bot commented Feb 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.09%. Comparing base (3ce1f08) to head (a493e97).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #719      +/-   ##
==========================================
+ Coverage   71.85%   72.09%   +0.24%     
==========================================
  Files          16       17       +1     
  Lines        3425     3444      +19     
==========================================
+ Hits         2461     2483      +22     
+ Misses        964      961       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@francescalb francescalb left a comment

Choose a reason for hiding this comment

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

Looks good as far as I was able to see when testing. However, tests are missing. Maybe add a test where testonto.ttl is squash saved and then check that this is the same as the original after loading? First check that new IRI and BASE_IRI are changed and change back of course...
And also check that the squashed ontology no longer imports other ontologies (Is this correct?)

Also, where is / --> # hack done? Is it a good idea to move this fix out of squash?

@jesper-friis
Copy link
Collaborator Author

Looks good as far as I was able to see when testing. However, tests are missing. Maybe add a test where testonto.ttl is squash saved and then check that this is the same as the original after loading? First check that new IRI and BASE_IRI are changed and change back of course... And also check that the squashed ontology no longer imports other ontologies (Is this correct?)

Also, where is / --> # hack done? Is it a good idea to move this fix out of squash?

Added a second test with squash and change of IRIs to tests/tools/test_ontoconvert.py

There is no explicit / --> # hack. Instead the --iri and --base-iri options of the ontoconvert tool allow setting the iri and base_iri after the ontology is loaded. Hence, these options can be used to work around the bug in Owlready2.

@francescalb
Copy link
Collaborator

Looks good as far as I was able to see when testing. However, tests are missing. Maybe add a test where testonto.ttl is squash saved and then check that this is the same as the original after loading? First check that new IRI and BASE_IRI are changed and change back of course... And also check that the squashed ontology no longer imports other ontologies (Is this correct?)
Also, where is / --> # hack done? Is it a good idea to move this fix out of squash?

Added a second test with squash and change of IRIs to tests/tools/test_ontoconvert.py

There is no explicit / --> # hack. Instead the --iri and --base-iri options of the ontoconvert tool allow setting the iri and base_iri after the ontology is loaded. Hence, these options can be used to work around the bug in Owlready2.

So the bug is still there for any other way of saving the ontology (i.e. not using ontoconvert)?

Copy link
Collaborator

@francescalb francescalb left a comment

Choose a reason for hiding this comment

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

Thank you for updating the test. Since you have decided not to use pylint eand fixtures a clean up of that is neded.

@jesper-friis
Copy link
Collaborator Author

Looks good as far as I was able to see when testing. However, tests are missing. Maybe add a test where testonto.ttl is squash saved and then check that this is the same as the original after loading? First check that new IRI and BASE_IRI are changed and change back of course... And also check that the squashed ontology no longer imports other ontologies (Is this correct?)
Also, where is / --> # hack done? Is it a good idea to move this fix out of squash?

Added a second test with squash and change of IRIs to tests/tools/test_ontoconvert.py
There is no explicit / --> # hack. Instead the --iri and --base-iri options of the ontoconvert tool allow setting the iri and base_iri after the ontology is loaded. Hence, these options can be used to work around the bug in Owlready2.

So the bug is still there for any other way of saving the ontology (i.e. not using ontoconvert)?

Yes, it is still there. Issue #723 is a suggestion to work around it when loading an ontology.

ontopy/testutils.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@francescalb francescalb left a comment

Choose a reason for hiding this comment

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

Approved, just a minor typo to be fixed.

Co-authored-by: Francesca L. Bleken <48128015+francescalb@users.noreply.github.com>
@jesper-friis jesper-friis merged commit be27825 into master Feb 29, 2024
12 checks passed
@jesper-friis jesper-friis deleted the save-squashed-ontology branch February 29, 2024 07:49
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