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

Ontology to pycram objects #191

Closed
wants to merge 5 commits into from
Closed

Ontology to pycram objects #191

wants to merge 5 commits into from

Conversation

mpomarlan
Copy link

Work in progress, but started a pull request to draw attention to this.

TODOs:

  1. SERIOUS: get SOMA_DFL and/or SOMA_DFL_module online so the onto loader can get to them without needing to depend on the dfl python package. Currently, these would be accessible only on a github tree.
  2. Serious: test to remove silly typos
  3. Good to have (but not threatening to functionality, at least in limited use cases): Integration with rest of onto manager. E.g., I prefer to have an iri2Concept map to switch between string names and the actual OWLReady2 generated classes. What other functions in the onto manager may impact this mapping?
  4. Good to have (eventually): query complex concept expressions. This will require integrating a different reasoner in there (Konclude) or only use modules of SOMA_DFL so that OWLReady2's HermiT doesn't die.
  5. Good to have (eventually): a more elegant map between pycram object types and ontology classes.

@@ -813,3 +874,60 @@ def reason(self, world: OntologyWorld = None, use_pellet_reasoner: bool = True)
return False
rospy.loginfo(f"{reasoner_name} reasoning finishes!")
return True

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also provide tests for such methods. Furthermore stick to the RST docstrings.

Returns:
conceptIRI: an IRI string. If the input conceptName is already an IRI form, conceptIRI = conceptName.
:param conceptName: a string containing a possibly shortened concept name
:type conceptName: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

types are not needed since they are annotated in the method signature

@@ -69,7 +71,9 @@ def remove_sql_file(cls, sql_filepath: str):
if os.path.exists(sql_journal_filepath):
os.remove(sql_journal_filepath)

@unittest.skipUnless(True, 'never skip')
Copy link
Collaborator

Choose a reason for hiding this comment

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

this line does nothing. Remove it

@@ -303,6 +307,52 @@ def test_ontology_save(self):
self.assertTrue(self.ontology_manager.save(owl_filepath))
self.assertTrue(Path(owl_filepath).is_file())
self.assertTrue(Path(sql_filepath).is_file())

@unittest.skipUnless(True, 'Never skip')
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

@@ -116,7 +119,7 @@ def test_loaded_ontologies(self):
self.assertIsNotNone(self.main_ontology)
self.assertTrue(self.main_ontology.loaded)
if self.ontology_manager.main_ontology_iri is SOMA_ONTOLOGY_IRI or \
self.ontology_manager.main_ontology_iri is SOMA_HOME_ONTOLOGY_IRI:
self.ontology_manager.main_ontology_iri is SOMA_DFL_ONTOLOGY_IRI:
Copy link

Choose a reason for hiding this comment

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

Suggested change
self.ontology_manager.main_ontology_iri is SOMA_DFL_ONTOLOGY_IRI:
self.ontology_manager.main_ontology_iri is SOMA_HOME_ONTOLOGY_IRI or \
self.ontology_manager.main_ontology_iri is SOMA_DFL_ONTOLOGY_IRI:

Copy link

@Tadinu Tadinu left a comment

Choose a reason for hiding this comment

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

This test is meant for any SOMA onto actually

@tomsch420 tomsch420 closed this Nov 7, 2024
@tomsch420
Copy link
Collaborator

Closing this due to inactivity

@mpomarlan
Copy link
Author

Ping. Inactivity because I wasn't sure what to do. Were there any changes requested?

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.

3 participants