[Renaming] SetActionMask -> SetDiscreteActionMask + added the virtual method CollectDiscreteActionMasks #3525
Conversation
vincentpierre changed the title 
| /// <returns>A mask for the agent. A boolean array of length equal to the total number of | ||
| /// actions.</returns> | ||
| internal bool[] GetMask() | ||
| internal bool[] GetDiscreteActionMask() |
There was a problem hiding this comment.
I'm OK leaving this as GetMask(). The "DiscreteAction" part in the class name seems like enough
There was a problem hiding this comment.
You think? Why is it different from SetMask()?
There was a problem hiding this comment.
I just think it's redundant. DiscreteActionMasker.GetMask() and DiscreteActionMasker.GetDiscreteActionMask() convey the same amount of info. (same for Set...)
Co-Authored-By: Chris Elion <chris.elion@unity3d.com>
Co-Authored-By: Chris Elion <chris.elion@unity3d.com>
Co-Authored-By: Chris Elion <chris.elion@unity3d.com>
I will leave this decision to the discretion of the reviewers |
| /// the utilities for setting and retrieving them. | ||
| /// </summary> | ||
| public class ActionMasker | ||
| public class DiscreteActionMasker |
There was a problem hiding this comment.
Doesn't the file name need to change too?
com.unity.ml-agents/Runtime/Agent.cs Outdated
| /// to the agent in addition to the vector observations implemented by the | ||
| /// user in <see cref="Agent.CollectObservations(VectorSensor)"/> or | ||
| /// <see cref="Agent.CollectObservations(VectorSensor, ActionMasker)"/>. | ||
| /// <see cref="Agent.CollectObservations(VectorSensor, DiscreteActionMasker)"/>. |
There was a problem hiding this comment.
This overload no longer exists, right?
| Re-requesting reviews because I changed the names again |
com.unity.ml-agents/Runtime/Agent.cs Outdated
| /// Visual observations are implicitly added from the cameras attached to | ||
| /// the Agent. | ||
| /// When using Discrete Control, you can prevent the Agent from using a certain | ||
| /// action by masking it. You can call the following method on the ActionMasker |
There was a problem hiding this comment.
This still feels a bit redundant; how about something like
/// When using Discrete Control, you can prevent the Agent from using a certain /// action by masking it with <see cref="DiscreteActionMasker.SetMask(int, IEnumerable{int})"/> (Remove the "The first argument..." part, since the DiscreteActionMasker docs should cover that.)
Co-Authored-By: Chris Elion <chris.elion@unity3d.com>
3 participants
Proposed change(s)
Rename and refactor the ActionMasker class
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
Types of change(s)
Checklist
Other comments