[Renaming] SetActionMask -> SetDiscreteActionMask + added the virtual method CollectDiscreteActionMasks #3525
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vincentpierre
changed the title
surfnerd
approved these changes
Feb 27, 2020
chriselion
reviewed
Feb 27, 2020
chriselion
reviewed
Feb 27, 2020
| @@ -110,7 +74,7 @@ public void SetActionMask(int branch, IEnumerable<int> actionIndices) | |||
| /// </summary> | |||
| /// <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...)
chriselion
reviewed
Feb 27, 2020
chriselion
reviewed
Feb 27, 2020
chriselion
approved these changes
Feb 27, 2020
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 |
chriselion
reviewed
Feb 28, 2020
| @@ -11,7 +11,7 @@ namespace MLAgents | |||
| /// left side of the board). This class represents the set of masked actions and provides | |||
| /// 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?
chriselion
reviewed
Feb 28, 2020
com.unity.ml-agents/Runtime/Agent.cs
Outdated
| @@ -60,7 +60,7 @@ internal struct AgentAction | |||
| /// environment. Observations are determined by the cameras attached | |||
| /// 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 |
chriselion
reviewed
Feb 28, 2020
com.unity.ml-agents/Runtime/Agent.cs
Outdated
| /// this method is called and the resulting size of the vector observation | ||
| /// needs to match the vectorObservationSize attribute of the linked Brain. | ||
| /// 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.)
chriselion
reviewed
Feb 28, 2020
chriselion
approved these changes
Feb 28, 2020
Co-Authored-By: Chris Elion <chris.elion@unity3d.com>
surfnerd
approved these changes
Feb 28, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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