Skip to content
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

Method to return stacked observations #5547

Merged
merged 4 commits into from
Oct 25, 2021
Merged

Method to return stacked observations #5547

merged 4 commits into from
Oct 25, 2021

Conversation

cmard
Copy link
Contributor

@cmard cmard commented Sep 23, 2021

Proposed change(s)

Agent can now access the stacked vector observations by calling GetStackedObservations() #5523

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

@CLAassistant
Copy link

CLAassistant commented Sep 23, 2021

CLA assistant check
All committers have signed the CLA.

@@ -13,6 +13,7 @@ and this project adheres to
### Minor Changes
#### com.unity.ml-agents / com.unity.ml-agents.extensions (C#)
- Added the capacity to initialize behaviors from any checkpoint and not just the latest one (#5525)
- Agent can now view the StackedVectorObservations (#5523)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you change this to read:

"Added the ability to get a read-only view of the stacked observations (#5523)

@@ -279,5 +286,20 @@ public BuiltInSensorType GetBuiltInSensorType()
IBuiltInSensor wrappedBuiltInSensor = m_WrappedSensor as IBuiltInSensor;
return wrappedBuiltInSensor?.GetBuiltInSensorType() ?? BuiltInSensorType.Unknown;
}

/// <summary>
/// Returns a read-only view of the observations that added.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you update this to read:

"Returns the stacked observations as a read-only collection.

/// <summary>
/// Returns a read-only view of the observations that added.
/// </summary>
/// <returns>A read-only view of the observations list.</returns>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you update this to read:

"The stacked observations as a read-only collection."

@cmard cmard force-pushed the stacked-observations branch from 64177d4 to 80e4fe8 Compare October 19, 2021 19:29
@cmard cmard requested a review from miguelalonsojr October 19, 2021 20:02
Copy link
Collaborator

@miguelalonsojr miguelalonsojr left a comment

Choose a reason for hiding this comment

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

LGTM.

Comment on lines 39 to 43
//[
//[1,2]
//[3,4]
//[5,6]
//]
Copy link
Contributor

Choose a reason for hiding this comment

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

are these comments needed?

@cmard cmard merged commit 9160d85 into main Oct 25, 2021
@delete-merged-branch delete-merged-branch bot deleted the stacked-observations branch October 25, 2021 14:38
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2022
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