-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
C# changes for hybrid action spaces #4587
C# changes for hybrid action spaces #4587
Conversation
* Add test env for hybrid actions, clean up BehaviorSpec * Add reward function and proper step
…hnologies/ml-agents into develop-hybrid-actions
BrainName = name, | ||
IsTraining = isTraining | ||
}; | ||
if (bp.VectorActionDescriptions != null) | ||
{ | ||
brainParametersProto.VectorActionDescriptions.AddRange(bp.VectorActionDescriptions); | ||
brainParametersProto.VectorActionDescriptionsDeprecated.AddRange(bp.VectorActionDescriptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't think we should deprecate ActionDescriptions (unless we're replacing them with more specific ContinuousActionDescriptions , etc). We don't currently use them, but I think they will be helpful for debugging in the future, especially now that we can have multiple actuators and action types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this because we won't use it anywhere other than deprecated cases since it's for VectorAction, and if we ever need a description we should add Continuous(/Discrete)ActionDescriptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I want to do a pass before we merge to master on a few things like the error messages, and whether we want "hybrid" to be the actual user-facing name. But that shouldn't block this for now.
com.unity.ml-agents/Runtime/Communicator/UnityRLCapabilities.cs
Outdated
Show resolved
Hide resolved
/// The Barracuda engine model for loading static parameters. | ||
/// </param> | ||
/// <returns>True if the model uses deprecated output fields.</returns> | ||
public static bool UseDeprecated(this Model model) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is future proof. There will be some more deprecations in the future. Maybe using a model version or calling this NoHybridSupport
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but don't like the negative term. I changed it to HasHybridSupport()
.
* separate entries for continuous/discrete in action proto * store actions in AgentInfo as ActionBuffers instead of arrays
2244680
to
ea6393b
Compare
* fix export node missing bug by forcing different nodes for new and deprecated action output * fix dynamic axis
…ity-Technologies/ml-agents into develop-hybrid-actions-csharp
Proposed change(s)
C# changes for hybrid action spaces
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
Types of change(s)
Checklist
Other comments