Improved representation of blank nodes

[jesper-friis]

Description:

• Changed get_unabbriviated_triples() to represent blank nodes such they can be used for reasoning.
• Has not changed the result of Ontology.__eq__().
• Added get_unabbriviated_triples() to World too, which will return all triples in the world.

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]

Looks nice. Smart to put the get_unabbreviated_triples in world.

[CasperWA]

Cheers @jesper-friis.

I'd like to propose writing out BNode to BlankNode to not confuse with "binary" or something else.

I've tried to add a bit more description to it as well, feel free to adapt it as you please.

One final thing: It's quite unclear to me what blank_node can be in the get_unabbreviated_triples() methods. Should it be a string (as you do in __eq__() or should it be a BlankNode type? I'm pretty sure some of the code only works with one or the other - specifically the internal _unabbreviate() function.

[CasperWA]

Suggested change

	"""Represents a blank node."""
	"""A blank node.
	A blank node is an ontology triple without any explicit content.
	It is similar to the concept of programmatically allocating memory for a variable by first initiating it as an empty variable of the desired type.
	"""

[jesper-friis]

A bnode is a node, not a triple. I have pushed an updated description.

[CasperWA]

Suggested change

	return BNode(self, i)
	return BlankNode(self, i)

[CasperWA]

Re-adding the now outdated comments from my recent review.

[jesper-friis]

One final thing: It's quite unclear to me what blank_node can be in the get_unabbreviated_triples() methods. Should it be a string (as you do in __eq__() or should it be a BlankNode type? I'm pretty sure some of the code only works with one or the other - specifically the internal _unabbreviate() function.

The idea is to represent blank nodes as BNode objects. However, since bnodes are local, it becomes tricky to compare them across imported ontologies. The purpose of introducing the bnode argument of _unabbriveate() is to allow __eq__() to represent all bnodes in a similar manner. Anything could have been used, but a string is just simple and fast. Since it is internal to the __eq__() function, it doesn't affect anything outside it.

[francescalb]

@CasperWA @jesper-friis Please decide on whether we should have bnode or blank_node as variable, just make sure that we are consistent. I do not really have very strong preferences. I agree that blank_node is more understandable, but I do not like to long variable names as I find codes more difficult to read when they become long. The reason the tests fails now is because bnode is only changed to blank_node in one place.

[jesper-friis]

I tried to accept Casper's suggestions about renaming bnode to blank_node. The last inconsistencies are now removed. @CasperWA, are you happy to merge now?

I kept bnode as the argument name of get_unabbreviated_triples() since it not intended to be an instance of BlankNode. Probably something as generic as s would be an even better naming of the argument.

[CasperWA]

I kept bnode as the argument name of get_unabbreviated_triples() since it not intended to be an instance of BlankNode. Probably something as generic as s would be an even better naming of the argument.

Yes, please update this to subject, entity or similar. Then I think this should be good to go. Let me just do a proper review after this change first, though.

[jesper-friis]

Done. Changed "label"

[CasperWA]

Thanks @jesper-friis - I'll merge this in now.