Skip to content

Conversation

@chriselion
Copy link
Contributor

Proposed change(s)

Add unit tests for SensorShapeValidator, fix potential errors.

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

@chriselion chriselion changed the title Develop sensor validator test Add SensorShapeValidator unit Feb 26, 2020
// TODO make sure this only checks once per agent
Debug.Assert(m_SensorShapes.Count == sensors.Count, $"Number of Sensors must match. {m_SensorShapes.Count} != {sensors.Count}");
for (var i = 0; i < m_SensorShapes.Count; i++)
for (var i = 0; i < Mathf.Min(m_SensorShapes.Count, sensors.Count); i++)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If these differed before, we'd get the assert, but would potentially continue and iterate through the lists, so accessing sensors[i] would read past the end of the list.

string m_Name = "NoopSensor";
int[] m_Shape;

public NoopSensor(int dim1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just some convenience methods for making sensors of a certain size.


namespace MLAgents.Tests
{
public class NoopSensor : ISensor
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a Noop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can rename to DummySensor or something clearer

Copy link
Contributor

Choose a reason for hiding this comment

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

NoOpSensor?

@chriselion chriselion merged commit 3e49abb into master Feb 27, 2020
@delete-merged-branch delete-merged-branch bot deleted the develop-sensor-validator-test branch February 27, 2020 19:16
@chriselion chriselion mentioned this pull request Feb 27, 2020
10 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 16, 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.

4 participants