fixed grabbing behaviour in agent #1148
Conversation
agents.py
Outdated
| elif action == 'Grab': | ||
| things = [thing for thing in self.list_things_at(agent.location)] | ||
| if agent.can_grab(things[0]): | ||
| if things: |
There was a problem hiding this comment.
This line if things should be on the top otherwise things[0] (accessed in the line above) would raise an exception!
agents.py
Outdated
| # for obj in thing.holding: | ||
| # super().delete_thing(obj) | ||
| # for obs in self.observers: | ||
| # obs.thing_deleted(obj) |
There was a problem hiding this comment.
I don't see a reason to comment these lines out. Even though the second line is redundant, the observers of the environment still need to be informed about each thing being deleted.
There was a problem hiding this comment.
if we delete all of the agent holdings with del thing.holding, and thanks to python memory management. All instants of the agent's holding would vanish from memory even in the observer list. Tell me what you think.
There was a problem hiding this comment.
The observers of the environment need to be updated about all the things getting deleted along with the agent. I have not seen observers used anywhere, so it depends upon the maintainers of the project to keep or exclude it!
|
I think this will also need some tests to be added |
Ahh I realized that after making the pull request, the tests would be easy I guess. |
|
could anyone help me understand why the checks have failed? It fails for python3.4 only |
It has nothing to do with the pr. Just close and reopen and the tests will hopefully pass... |
|
Turns out pyyaml has removed support for python 3.4. This is a issue for keras. We don't need to worry about it. |
|
Here's the upstream issue on keras. gh-13674 |
|
I guess it is better if I open an issue. |
|
@Okhaledzaki I don't think this is our issue but I will leave it upto the maintainers to decide! |
agents.py
Outdated
| @@ -967,21 +967,16 @@ def execute_action(self, agent, action): | |||
|
|
|||
| agent.bump = False | |||
| if action == 'TurnRight': | |||
There was a problem hiding this comment.
I think the if statement could be streamlined if we rewrote it as:
if action in ['TurnRight', 'TurnLeft', 'Forward']:
super().execute_action(agent,action)
agent.performance -= 1
elif action == 'Grab':
super().execute_action(agent,action)
I think the above looks a bit better.
There was a problem hiding this comment.
I had that in mind, but I thought to myself that being more articulate and reasonably redundant in code would help the student retaining the information that is my case.
There was a problem hiding this comment.
But actually seeing it, it really didn't help much.
agents.py
Outdated
| super().execute_action(agent,action) | ||
| agent.performance -= 1 | ||
| elif action == 'Grab': | ||
| if action in ['TurnRight', 'TurnLeft', 'Forward','Grab']: |
There was a problem hiding this comment.
A space is needed after the last comma.
|
something has broken in the deeplearning module @antmarakis :"D |
|
@antmarakis The tests have all passed finally phew |
agents.py
Outdated
| if len(things): | ||
| agent.holding.append(things[0]) | ||
| if action in ['TurnRight', 'TurnLeft', 'Forward', 'Grab']: | ||
| super().execute_action(agent,action) |
There was a problem hiding this comment.
You need a space after the comma, to comply with the overall project style.
When a thing is grabbed, it would be hidden from the environment. When released or the agent is removed it would be added to environment again at the same location of the agent. For the issue of grabbed thing movement, it is already implemented. No test has failed.
Have I missed something?