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

Added function to load the emmo (the ontology) directly #226

Merged
merged 6 commits into from
Sep 24, 2021

Conversation

francescalb
Copy link
Collaborator

@francescalb francescalb commented Sep 22, 2021

Closes #209.

We could call it get_emmo instead.

Copy link
Collaborator

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

This is quite nice.
My comments are mainly for some typing tricks and such, not really the content.

Another thing I was thinking though was whether it should be named in capitals, where possible, instead of the lower case letters?

emmopy/emmopy.py Outdated Show resolved Hide resolved
emmopy/emmopy.py Outdated Show resolved Hide resolved
emmopy/emmopy.py Outdated Show resolved Hide resolved
emmopy/emmopy.py Outdated Show resolved Hide resolved
tests/test_load_emmo.py Outdated Show resolved Hide resolved
@CasperWA
Copy link
Collaborator

We could call it get_emmo instead.

Not a bad idea, it's still a bit confusing I think.
Mainly I was thinking to have something like:

from emmopy import EMMO

Which would mean we could simply create an EMMO variable in the __init__.py file, or like you've done here, in a separate file (which is a bit nicer), and then import it in __init__.py. The trouble being that one will always instantiate the EMMO ontology then when importing anything from emmopy. Unless it's done with some lazy loading tricks.

What I'm trying to say is, it might be best to rename to get_emmo() and then let users use this to create their own EMMO variables, like:

from emmopy import get_emmo

EMMO = get_emmo(inferred=False)
EMMO_inferred = get_emmo()

@francescalb
Copy link
Collaborator Author

We could call it get_emmo instead.

Not a bad idea, it's still a bit confusing I think.
Mainly I was thinking to have something like:

from emmopy import EMMO

Which would mean we could simply create an EMMO variable in the __init__.py file, or like you've done here, in a separate file (which is a bit nicer), and then import it in __init__.py. The trouble being that one will always instantiate the EMMO ontology then when importing anything from emmopy. Unless it's done with some lazy loading tricks.

What I'm trying to say is, it might be best to rename to get_emmo() and then let users use this to create their own EMMO variables, like:

from emmopy import get_emmo

EMMO = get_emmo(inferred=False)
EMMO_inferred = get_emmo()

I think get_emmo is better, since using from emmopy import EMMO would increase the risk of overwriting with EMMO = EMMO() later.

emmopy/emmopy.py Outdated Show resolved Hide resolved
tests/test_load_emmo.py Show resolved Hide resolved
Good suggestions from Casper.

Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
Copy link
Collaborator

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Merge at will if/when tests pass.

Copy link
Collaborator

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Par excellence

@francescalb francescalb merged commit 3c5ff38 into master Sep 24, 2021
@francescalb francescalb deleted the flb/close-209-load-emmo_ontology branch September 24, 2021 14:54
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.

Make function that automatically loads emmo
2 participants