Added option: EMMObased = False in ontology.load()

[francescalb]

Description:

Loading ontologies that do not load skos and rdfs-schema needs to be supported.

Type of change:

• Bug fix.
• New feature.
• Documentation 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:

[francescalb]

@jesper-friis I suggest having an option for loading that is EMMObased=False, with True as default.
Alternatively populated DAFAULT_LABEL_ANNOTATIONS in some way if they are in the ontology.

I am opening this as a draft pull request so we can discuss.

[jesper-friis]

I like the idea of having an option about whether an ontologi is EMMO-based or not. Maybe we even can generalise it further? What about making an settings data class, that collects all settings in one place? We can then offer two predefine settings selectable via a "style" (do we have a better name) option; a plain default style for ontopy and a emmo-style for emmopy. Users then either modify one of the pre-defined styles for their needs or create their own style.

I valid question is whether it is worth the effort. I actually think it might be, both because I think it would easy to implement and because it could simplify the rest of the code by moving logics about default options to a dedicated module.

[francescalb]

That sounds like a good idea, but are there more styles you are thinking about?

DEFAULT_LABEL_ANNOTATIONS
name_policy='uuid' in sync_attributes

[jesper-friis]

For our current needs, I don't think we need more than the 'plain' and 'emmo' style. But if people from e.g. the OBO community wants to use ontopy, it could be convenient for them to define a 'obo' style.

[jesper-friis]

This looks good. I think the possible generalisation discussed in this PR should be a separate issue.

[francescalb]

Sure. I meant what more than DEFAULT_ANNOTATION_LABELS and name_policy should be included in the style?

[jesper-friis]

Sorry for the misunderstanding. Some candidates in Ontology:

• the class attributes dir_preflabel, dir_label, dir_name and dir_imported
• default arguments to load(): catalog_file, url_from_catalog
• default argument to sync_python_names(): annotations
• default argument to sync_python_reasoner(): reasoner
• default argument to sync_attributes(): name_policy, name_prefix, class_docstring

In ontograph.py it would be great to factor out the _default_style class attribute. ontodoc.py has also a lot of style-related code that would be great to factor out.

I think the new style or settings module should support separate sections for the different modules.

[francescalb]

OK, I have added tests for this simplified solution for now and opene a new issue for the more extensive style preferences

[francescalb]

@CasperWA
Test in test_modules.py fails because of redirection error in emmo-repo, Emanuele has been contacted about this.

[CasperWA]

@CasperWA Test in test_modules.py fails because of redirection error in emmo-repo, Emanuele has been contacted about this.

If you think it is okay to skip the tests until then, please do this (put a @pytest.mark.skip("this is a reason why this test is being skipped") as the decorator for the test functions that should be skipped).
Then add an issue to keep track of this so we can unskip it once the issue has been resolved.

In other words - one should never merge a PR that doesn't pass the CI.

[CasperWA]

Looks good.

I have some suggested changes, the only one I don't feel strongly about is the variable name change.

Concerning the test, please make a new function for this particular test instead of including it in another test function. The function name must start with test_ but that's it.

[francescalb]

@CasperWA Test in test_modules.py fails because of redirection error in emmo-repo, Emanuele has been contacted about this.

If you think it is okay to skip the tests until then, please do this (put a @pytest.mark.skip("this is a reason why this test is being skipped") as the decorator for the test functions that should be skipped). Then add an issue to keep track of this so we can unskip it once the issue has been resolved.

In other words - one should never merge a PR that doesn't pass the CI.

I was not thinking to merge it with a failing test. This is an old test and I expected the github-repo link to be fixed pretty fast as was the case :) My comment was to explain why I asked for a review even though there was an old test failing.

[CasperWA]

Rename the test_load() function in the test_load_rdf_rdfs.py file to test_load_rdfs() (or similar) and move it to the test_load.py file. There's no reason to create a new file.

[francescalb]

Rename the test_load() function in the test_load_rdf_rdfs.py file to test_load_rdfs() (or similar) and move it to the test_load.py file. There's no reason to create a new file.

Ah, so you want to have two tests in the same file. I understood that you wanted two separate files. Done now.

[CasperWA]

It all looks good to me now :) Thanks @francescalb !