Implementation of PseudoSensor #33
Conversation
…s and publishes information to the observation_space. still some features missing. goal observation not implemented yet. PseudoDistSensor not implemented yet. fixed setSpaces issue in pointRobotEnv when calling resetLimits()
… is able to track either the position or the distance to a goal. an extra sensor is implemented for sensing the closest n objects around. Testing is still necessary. Implementation should be changed to be consistent
|
Thanks a lot for the input. I will work on your change requests and let you know as soon as I have implemented the requested changes. |
…sensors.py needs to be deleted in future commit.
…ct class and obstacle and goal sensor; bug fix of resetfunction for resetLimits; reset example script to old version; sensor flag defines if sensors are added
|
Hi @maxspahn I included the changes and merged the new master branch into this feature branch. Could you check the updated PR? |
There was a problem hiding this comment.
Great work! I tested it (only with the point robot so far) and it works like a charm!
I made some comments mostly about variable naming and capitalized letters.
In the current stage, this implementation only supports point robots, as the forward kinematics is assumed to be the states of the robot. When experimenting with other kinematic chains, this implementation will genarilize to all robots.
planarenvs/planarCommon/planarEnv.py
Outdated
| @@ -76,6 +85,8 @@ def reset(self, pos=None, vel=None): | |||
| def step(self, a): | |||
| self.action = a | |||
| self.integrate() | |||
| for sensor in self._sensors: | |||
| self.state[sensor.name()] = sensor.sense(self.state, self._goals, self._obsts, self.t()) | |||
There was a problem hiding this comment.
The function call is great and exactly as I hoped it would be. Passing self._goals and self._obsts seems like the better solution here.
I would prefer to not add it to self.state as it might lead to trouble in the integration scheme. Could make a second attribute to the planarEnv class that stored sensed information? You then only concatenate them in the function self._get_ob().
There was a problem hiding this comment.
@maxspahn where do you see the potential conflict? I can split it into another state of course (e.g. sensorState). However is the Sensor information not also part of the state space?
There was a problem hiding this comment.
The state is passed to the integration scheme. At some future point, we might want to check the size of the state that is passed into the continuous dynamic (or the integrated one) to raise errors if something is going wrong there. Such an assertion would fail if the size of the state is different depending on how many sensors there are.
For me, the state is the state of the robot, although I know that this is not really true in the RL-community.
There was a problem hiding this comment.
Okay I understand. I thought it might be sufficient, since it is easy to access the relevant keys in the state now using 'x' or 'xdot' as it is a dictionary.
Should I go with the option to include an extra "sensorState" variable? I would then add the "state" with the "sensorState" in the function _get_ob() in order to return the observation.
There was a problem hiding this comment.
Exactly, just add the two dictonaries in the the _get_ob() function and check there if the observation is contained in the observation space.
There was a problem hiding this comment.
Perfect. Should be done with the last commit. Could you check, if this is what you meant? I tested it for both with and without sensor and it works as I intended. Would be great if you could doublecheck though. Thank you!
sensors/GoalSensor.py
Outdated
| if idx >= self._nbObservations: | ||
| break | ||
| currGoalPos = goal.position(t=t) | ||
| currGoalDist = Dist2Circ(s['x'][0], s['x'][1], currGoalPos[0], currGoalPos[1], goal.epsilon()) |
There was a problem hiding this comment.
The variable s refers to the current state I assume. Maybe you can think of a more meaningful name here.
Then, you refer directly to the state as the forward kinematics of the point robot is the state essentially (configuration space equals workspace). Therefore, this does not work for other robots at the moment.
We need to either pass the forward kinematics to the sensor on initialize forward kinematics in the sensor constructor.
The second option seems better, as the goal object already contains information about which link the goal is defined for. We can discuss this later.
There was a problem hiding this comment.
Regarding the naming: Definetely!
kinematic chain: Exactly, right now it only works for the point robot. Would love to discuss how we can make this general for all robots!
There was a problem hiding this comment.
Let me know when you have it at a state that all the other issues and remarks are addressed. Then I will propose something to generalize it to generic kinematic chains.
…ys for the Obstacle Sensor
…ering; mostly changes in coding style; no changes in functionality
planarenvs/planarCommon/planarEnv.py
Outdated
| @@ -76,6 +85,8 @@ def reset(self, pos=None, vel=None): | |||
| def step(self, a): | |||
| self.action = a | |||
| self.integrate() | |||
| for sensor in self._sensors: | |||
| self.state[sensor.name()] = sensor.sense(self.state, self._goals, self._obsts, self.t()) | |||
…f we want to move to higher orders
… state. both are concatinated in the _get_ob() function and returnee as observation. if sensor flag is not set, the observation consists only of robot state x and xdot
Added implementation of PseudoSensor. PseudoSensor observes obstacles and publishes information to the observation_space. Still some features missing. goal observation not implemented yet. PseudoDistSensor not implemented yet. Fixed setSpaces issue in pointRobotEnv when calling resetLimits() (ref: #32)