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

Improved WM element (ID) handling #90

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

matthias-mayr
Copy link
Member

When testing, I ran into two issues that are addressed in this PR.

  1. The lightstring2uri method could return a string instead of an rdflib URIRef. This caused errors of this kind:
/wm: AauSpatialReasoner  warn: Adding relation to http://www.inf.ufrgs.br/phi-group/ontologies/cora.owl#Robot-1-skiros:test_robot
/wm: statement to add ('http://www.inf.ufrgs.br/phi-group/ontologies/cora.owl#Robot-1', rdflib.term.URIRef('http://www.w3.org/1999/02/22-rdf-syntax-ns#type'), rdflib.term.URIRef('http://www.w3.org/2002/07/owl#Named
  ~  Individual'))
/test_robot: Service call failed: service [/wm/modify] responded with an error: b'error processing request: Subject http://www.inf.ufrgs.br/phi-group/ontologies/cora.owl#Robot-1must be an rdflib term'
  1. When obtaining the ID number from an element, it was assumed that there is no dash "-" other than the separator between type and ID in the URI.
    Since we have URIs like http://www.inf.ufrgs.br/phi-group/ontologies/cora.owl#Robot-1, this could lead to errors like:
File "/home/matthias/Workspaces/skiros2_noetic_ws/src/skiros2/skiros2_world_model/src/skiros2_world_model/core/world_model.py", line 557, in _uri2id\n    return int(uri.split
   ~  (\'-\')[1])\n', "ValueError: invalid literal for int() with base 10: 'group/ontologies/cora.owl#Robot'\n"]
/test_robot: Service call failed: service [/wm/modify] responded with an error: b"error processing request: invalid literal for int() with base 10: 'group/ontologies/cora.owl#Robot'"

This two commits address both. However, I am not sure if they would cause side effects.
@frovida: If you have sth to comment, please do so.

I will merge them into the ROS 2 branch, so they would get some sort of testing.
This also came up when "reviving" the skiros2_test_lib. Once it's fully up, we can also make changes with some more confidence.

Previously this function could return strings, which caused an instance
check in rdflib to fail with "must be an rdflib term".
Previously obtaining the ID of URI's like:
"http://www.inf.ufrgs.br/phi-group/ontologies/cora.owl#Robot-186"
Failed, because it was assumed that "-" only appears once in the URI.
Now it searches only after the hash "#" if one exists
@matthias-mayr
Copy link
Member Author

wrong mention @frvd

@@ -150,9 +150,13 @@ def getIdNumber(self):
"""
@brief Return the element id number as integer
"""
if self._id.find('-') < 0:
hash_pos = self._id.find('#')
Copy link
Contributor

Choose a reason for hiding this comment

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

@matthias-mayr I think it would be just easier to replace find with rfind :)

Copy link
Member Author

Choose a reason for hiding this comment

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

rfind for the "-" or the "#"? Assume the former since the latter does not seem to make any sense to me

However just using rfind for the "-" does not necessarily yield to the expected result.
Example:
A malformed URI like http://www.inf.ufrgs.br/phi-group/ontologies/cora.owl#Robot that does not have an ID should lead to a return value of "-1" according to the previous/other code.
That's why I wanted to search explicitly after the hash if there's one

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, what about the following?

hash_pos = self._id.rfind('#')
dash_pos = self._id.rfind("-")
return -1 if dash_pos == -1 or dash_pos < hash_pos else int(self._id.split('-')[1])

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.

2 participants