Skip to content

Replaced get_behavior_names and get_behavior_spec with behavior_specs property #3946

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

Merged
merged 7 commits into from
May 12, 2020

Conversation

vincentpierre
Copy link
Contributor

Proposed change(s)

Replaced get_behavior_names and get_behavior_spec with behavior_specs property

Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)

Types of change(s)

  • Bug fix
  • New feature
  • Code refactor
  • Breaking change
  • Documentation update
  • Other (please describe)

Checklist

  • Added tests that prove my fix is effective or that my feature works
  • Updated the changelog (if applicable)
  • Updated the documentation (if applicable)
  • Updated the migration guide (if applicable)

Other comments

@vincentpierre vincentpierre self-assigned this May 11, 2020
@andrewcoh
Copy link
Contributor

Making it a property is much cleaner. Just curious as to the reason for creating BehaviorMapping. Is it just to have more explicit naming? From what I can tell it has the same functionality as a dictionary.

@vincentpierre
Copy link
Contributor Author

Making it a property is much cleaner. Just curious as to the reason for creating BehaviorMapping. Is it just to have more explicit naming? From what I can tell it has the same functionality as a dictionary.

Using Mapping allows me to have this property be read only.

@chriselion
Copy link
Contributor

Using Mapping allows me to have this property be read only.

Mapping is read-only (as opposed to MutableMapping), so using Mapping as the return type should be sufficient and you could just return self._env_specs in UnityEnvironment. It wouldn't be enforced at runtime, just by type checking and IDE syntax highlighting (but a user determined to mess things up could modify BehaviorMapping _dict too).

@vincentpierre
Copy link
Contributor Author

Mapping is read-only (as opposed to MutableMapping), so using Mapping as the return type should be sufficient and you could just return self._env_specs in UnityEnvironment. It wouldn't be enforced at runtime, just by type checking and IDE syntax highlighting (but a user determined to mess things up could modify BehaviorMapping _dict too).

Would you recommend dropping the BehaviorMapping alltogether?
I think it is safer to do it this way, it makes it harder to mess up (while not impossible) which is better in my opinion.

@chriselion
Copy link
Contributor

I think BehaviorMapping is overkill, but I'm not strongly opposed to it. I think having Mapping as part of the interface is the important part.

@@ -49,7 +49,7 @@ def __init__(
self._env = unity_env

# Take a single step so that the brain information will be sent over
if not self._env.get_behavior_names():
if len(self._env.behavior_specs) == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: pretty sure you can do if not self._env.behavior_specs

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. Any documentation/examples that need to change too?

@vincentpierre
Copy link
Contributor Author

Looks good. Any documentation/examples that need to change too?

Yes, coming soon

@vincentpierre vincentpierre requested a review from chriselion May 12, 2020 20:15
@vincentpierre
Copy link
Contributor Author

Looks good. Any documentation/examples that need to change too?

@chriselion Made the documentation changes.

@vincentpierre vincentpierre merged commit 266a53a into master May 12, 2020
@delete-merged-branch delete-merged-branch bot deleted the develop-env-new-spec branch May 12, 2020 22:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 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