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

Develop remove past action communication #2913

Merged
merged 19 commits into from
Nov 19, 2019

Conversation

vincentpierre
Copy link
Contributor

No description provided.

@vincentpierre vincentpierre self-assigned this Nov 15, 2019
@vincentpierre vincentpierre marked this pull request as ready for review November 19, 2019 00:48
@@ -172,7 +178,7 @@ def make_empty_memory(self, num_agents):
:param num_agents: Number of agents.
:return: Numpy array of zeros.
"""
return np.zeros((num_agents, self.m_size))
return np.zeros((num_agents, self.m_size), dtype=np.float)
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope for this PR, but should we try to make sure we never create float64 nparrays?

@@ -194,6 +200,34 @@ def remove_memories(self, agent_ids):
if agent_id in self.memory_dict:
self.memory_dict.pop(agent_id)

def make_empty_previous_action(self, num_agents):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any difference between how we handle memories and previous actions besides the the dtype? Worth combining into a small utility class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there is going to be a bigger refactor of this part coming later. I think it is fine for now. What do you think @ervteng ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth combining - but then again, there will only be two things we store between steps. Also, the "when" they're added is different - memories can be added after the get_action, previous_actions must be added before.

I think there is no way around it, this logic will still need to live in the Policy for Python inference to work. The only other place I can think of is the env_manager, but that would be messy.

@chriselion
Copy link
Contributor

Just to clarify, existing models will keep working as before? The only thing that would break when upgrading is existing demo files?

Copy link
Contributor

@chriselion chriselion left a comment

Choose a reason for hiding this comment

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

Looks good, optional to DRY up the python memory/action storage.

@vincentpierre
Copy link
Contributor Author

Models will still work as before but the demo files will not.

@ervteng
Copy link
Contributor

ervteng commented Nov 19, 2019

Models will still work as before but the demo files will not.

Will we need to re-record all of them or just the Hallway one?

@vincentpierre
Copy link
Contributor Author

Will we need to re-record all of them or just the Hallway one?

I reconverted them already

Copy link
Contributor

@ervteng ervteng left a comment

Choose a reason for hiding this comment

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

:shipit:

@vincentpierre vincentpierre merged commit 4f58e10 into develop Nov 19, 2019
@vincentpierre vincentpierre deleted the develop-remove-past-action-communication branch November 19, 2019 02:05
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants