Config bug: Trying to read type from embedder leads to exception for inverse_relations_model about kge HOT 8 CLOSED

You need to use get_option

from kge.

Look at all other models (e.g., rescal), they show how to access the config correctly.

from kge.

The self.get_option is a very hacky solution:

1. F.ex. in rescal and complex, config.set(self.configuration_key + ...) is used and it seems to be working but not config.get(self.configuration_key + ...).

2. self.get_option is only used in the KgeModels but not in other parts of the framework and from a user perspective its not clear why.

from kge.

1. Sure you can set something that has not been set before. Instead of get, use get_default (see the Config docs).

2. get_option is merely a shortcut to automatically add a configuration key. Original plan was to put it up to KgeBase, not sure anymore why this doesn't happen. @rufex2001?

from kge.

I am only talking about usability, and at them moment this is confusing and error prone.

A nice API would be:

1. get rid of get_option
2. make the config_key a separate kwarg for config.get_default and do the check from get_option there.
3. merge get and get_default
4. rename default to resolve_type and make a boolean argument get(..., resolve_type=True, ...) as it is done for set(..., create=True, ...). Then we have one get and one set with either an option to have a resolve_type/create behaviour or not.

from kge.

1+2. I prefer the current way better. No need to specify the config key everywhere.
3+4. Sounds good. Should be named just resolve or resolve_via_type.

I think this is really low priority. We should instead use get_option everywhere.

from kge.

Please open a new issue with the API changes. (This should be done carefully to not break any existing code.)

from kge.

1+2. I prefer the current way better. No need to specify the config key everywhere.

Correct, which is why it should be a optional argument.

I think this is really low priority.

I stumbled over this repeatedly. Now that I (unecessarily) understand the inner workings of config better and better it is low priority for me, but maybe not for others.

We should instead use get_option everywhere.

Also in the jobs?

from kge.