-
Notifications
You must be signed in to change notification settings - Fork 4.3k
step logic changes: unit test #3467
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
step logic changes: unit test #3467
Conversation
@@ -487,10 +487,8 @@ public void TestCumulativeReward() | |||
agent1.LazyInitialize(); | |||
agent2.SetPolicy(new TestPolicy()); | |||
|
|||
int expectedAgent1Resets= 0; |
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.
These were unused, and switch to var
instead of int
int expectedResets= 0; | ||
int expectedAgentAction = 0; | ||
int expectedAgentActionSinceReset = 0; | ||
var expectedAgentStepCount = 0; |
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.
Added some extra checks on the step count and collectobservations calls.
expectedAgentStepCount += 1; | ||
|
||
// If the next step will put the agent at maxSteps, we expect it to reset | ||
if (agent1.GetStepCount() == maxStep - 1 || (i == 0)){ |
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.
IMHO this is a little clearer (since it's a value on the actual Agent, not just a test variable)
Co-Authored-By: Chris Goy <goyenator@gmail.com>
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.
LGTM
* Moving the max step logic - Created a new Academy Event called AgentIncrementStep to be called before SetStatus - Implemented the AgentSteping logic * second commit : Moving the step counting at the begining. I had to edit the tests but I think they are now closer to what we want * addressing comments * Update com.unity.ml-agents/Runtime/Agent.cs Co-Authored-By: Chris Goy <goyenator@gmail.com> * Update com.unity.ml-agents/Runtime/Agent.cs Co-Authored-By: Chris Goy <goyenator@gmail.com> * Made the tests not be broken * Update com.unity.ml-agents/Runtime/Agent.cs Co-Authored-By: Chris Elion <chris.elion@unity3d.com> * step logic changes: unit test (#3467) * Added a line in the changelog Co-authored-by: Chris Goy <christopherg@unity3d.com> Co-authored-by: Chris Elion <celion@gmail.com>
To be merged into #3448