-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add owlready2-based OntologyConcept #141
Conversation
…logy concepts from SOMA DesignatorDescription add onto_concept attr as an owlready2 Thing ~Action auto fetch onto_concept from OntologyOWL
TestActionDesignatorGrounding add is-valid-onto_concept test
…tion: onto_concepts
src/pycram/designator.py
Outdated
@@ -361,6 +364,11 @@ def get_slots(self) -> List[str]: | |||
def copy(self) -> Type[DesignatorDescription]: | |||
return self | |||
|
|||
def get_default_onto_concept(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return type annotation
src/pycram/ontology.py
Outdated
""" | ||
|
||
""" | ||
The ontology instance as the result of an ontology loading operation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am fairly confident that docstrings for class variables should be below the variable declaration and not above. Please check that this is rendered as you intended by building the doc.
src/pycram/ontology.py
Outdated
owlready2.Property]] = owlready2.ObjectProperty): | ||
""" | ||
Dynamically create ontology triple classes under same namespace with the main ontology, | ||
aka subject, predicate, object, with relation among them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no abbreviations in docstrings also
src/pycram/ontology.py
Outdated
return obj_designator | ||
|
||
@classmethod | ||
def demo_placeable_on_candidates_query(cls): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
demos should be either testcases or jupyter notebooks in the documentation examples. Please remove all demos from production code.
src/pycram/ontology.py
Outdated
print(edible_onto_concept, [des.types for des in edible_onto_concept.designators]) | ||
|
||
|
||
if __name__ == "__main__": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pycram internal modules should not have entry points like this.
@@ -124,6 +125,7 @@ def __init__(self, names: List[str], types: List[str], | |||
:param reference_frames: Frame of reference in which the object position should be | |||
:param timestamps: Timestamps for which positions should be returned | |||
:param resolver: An alternative resolver that resolves the input parameter to an object designator. | |||
:param onto_concepts: A list of ontology concepts that the object is categorized as | |||
""" | |||
super(LocatedObject, self).__init__(names, types, resolver) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no onto_concepts here
src/pycram/helper.py
Outdated
@@ -22,6 +23,13 @@ | |||
import os | |||
import math | |||
|
|||
class Singleton(type): | |||
_instances = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here docstring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the instances dict still needs and explanation
Make owlready2 optional dependency Add test for OntologyManager
used to link an ontology concept, either created dynamically or loaded from ontology, with designators & custom resolving callable
examples/ontology.ipynb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the notebook prints thousands of info lines. Make sure that this is what you want to show in the end by building the doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, so I trimmed it down to showing several examples only
src/pycram/helper.py
Outdated
@@ -22,6 +23,13 @@ | |||
import os | |||
import math | |||
|
|||
class Singleton(type): | |||
_instances = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the instances dict still needs and explanation
@@ -0,0 +1,2 @@ | |||
-r requirements.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is necessary to add another requirements file just for one requirement, just put this in the default requirements.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since some may integrate another reasoning-based methods (eg: neural-network based), I'd say ontology should be an optional plugin to pycram core operations.
I actually added it in requirements.txt but then decided to move out as @tomsch420's suggestion.
@@ -61,6 +61,7 @@ jobs: | |||
cd /opt/ros/overlay_ws/src/pycram | |||
pip3 install -r requirements.txt | |||
pip3 install -r requirements-resolver.txt | |||
pip3 install -r requirements-ontology.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this and add the requirement to requirements.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subject to our consensus on whether to make Ontology integration as optional or not.
try: | ||
import owlready2 | ||
except ImportError: | ||
owlready2 = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add some logging here. This case should not happen if owlready2 is in requirements.txt but just to be sure.
src/pycram/designator.py
Outdated
""" | ||
Create a Designator description. | ||
|
||
:param resolver: The grounding method used for the description. The grounding method creates a location instance that matches the description. | ||
:param resolver: The grounding method used for the description. The grounding method creates a location instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc strings of Parameter have to be in one line, otherwise the autoAPI will not render them correctly.
src/pycram/designator.py
Outdated
@@ -437,14 +449,36 @@ def insert(self, session: Session, *args, **kwargs) -> ORMAction: | |||
|
|||
return action | |||
|
|||
def __init__(self, resolver=None): | |||
super().__init__(resolver) | |||
def __init__(self, resolver=None, ontology_concept_holders: Optional[List[owlready2.Thing]] = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The typing for ontology_concept_holders is different from the parent class, I guess this is still valid due to inheritance right?
def get_designators_of_ontology_concept(self, ontology_concept_name: str) -> List[DesignatorDescription]: | ||
""" | ||
Get the corresponding designators associated with the given ontology concept | ||
:param ontology_concept_name: An ontology concept name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All methods are missing doc strings for the return value of the function
src/pycram/ontology/ontology.py
Outdated
from pycram.helper import Singleton | ||
from pycram.designator import DesignatorDescription, ObjectDesignatorDescription | ||
|
||
from pycram.ontology.ontology_common import OntologyConceptHolderStore, OntologyConceptHolder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make Pycram imports relative
src/pycram/ontology/ontology.py
Outdated
rospy.loginfo(f"Direct Instances: {list(ontology_class.direct_instances())}") | ||
rospy.loginfo(f"Inverse Restrictions: {list(ontology_class.inverse_restrictions())}") | ||
|
||
def load_ontology(self, ontology_iri) -> tuple[owlready2.Ontology, owlready2.Namespace]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing type for ontology_iri
src/pycram/ontology/ontology.py
Outdated
return out_classes | ||
|
||
@staticmethod | ||
def get_ontology_class_by_ontology(ontology: owlready2.Ontology, class_name: str) -> Type[owlready2.Thing] | None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typing with "|"
src/pycram/ontology/ontology.py
Outdated
""" | ||
Create a designator that belongs to a given ontology concept class | ||
|
||
:param designator_name: Designator name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Designator name might be confusing since it implies that it is the name of the designator itself and not what the designator describe. object_name might be better suited.
Optional as only required if creating Object Designator
Added geometry2
Add owlready2-based OntologyConcept
Add owlready2-based OntologyConcept
This PR adds API that enables designators to link themselves with concepts defined in ontologies, specifically ones of OWL format, utilizing owlready2.
-> This will enable robot motion plan to receive abstract ontology concepts as inputs, which will resolve themselves to specific physical objects in world, upon certain conditions/constraints from user scenario requirements.
Main updates are:
ontology.py
with `OntologyManager, providing various queries into ontology classes and properties, as well as relations among them.ontology_common.py
withOntologyConceptHolder
wrapping ontology_concept as anowlready2.Thing
to avoid inheritance with added attributes that cannot be saved into an OWL file and reused later upon that file reloading.Notably,
DesignatorDescription, ObjectDesignatorDescription, ~ActionDesignatorDescription
: addonto_concept_holders
A list of holders ontology concepts that the designator type is semantically categorized as or associated withAdd
ontology.ipynb
, tutorial featuring basic usages ofOntologyManager
with examples used in robot motion planbullet_world_testcase.py, test_action_designator.py
: Add tests for ontology concepts loaded from SOMArequirements.txt
addowlready2
@AbdelrhmanBassiouny @tomsch420