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

Make CollectObservations take a VectorSensor #3352

Merged
merged 16 commits into from
Feb 7, 2020

Conversation

vincentpierre
Copy link
Contributor

No description provided.

@vincentpierre vincentpierre self-assigned this Feb 4, 2020
@chriselion
Copy link
Contributor

Seems OK. I'm not sure it actually makes the API any smaller though, just moves it to VectorSensor.

@vincentpierre
Copy link
Contributor Author

Yes, but it does simplify the Agent code (since it does not forward the calls to addobservation for every type of observation.

@chriselion
Copy link
Contributor

But it also couples the public interface of Agent more closely to VectorSensor, whereas before they were decoupled (and we could theoretically change the implementation of how AddVectorObs worked)

@chriselion
Copy link
Contributor

I retract my objections. Make it so!

@vincentpierre vincentpierre marked this pull request as ready for review February 6, 2020 19:45
docs/Migrating.md Outdated Show resolved Hide resolved
docs/Migrating.md Outdated Show resolved Hide resolved
vincentpierre and others added 7 commits February 6, 2020 12:50
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>
Co-Authored-By: Chris Elion <chris.elion@unity3d.com>

namespace MLAgents.Sensor
namespace MLAgents
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for this change? Should we (roughly) keep the namespaces 1:1 with the subdirectories?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but now the user would have to import both MLAgents and MLAgents.Sensors when implementing an Agent that has a CollectObservation implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All right. Removed the MLAgents.Sensor namespace alltogether

Copy link
Contributor

@chriselion chriselion left a comment

Choose a reason for hiding this comment

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

One question about the namespace, but looks good.

@chriselion
Copy link
Contributor

:shipit:

@vincentpierre vincentpierre merged commit 6551974 into master Feb 7, 2020
@delete-merged-branch delete-merged-branch bot deleted the develop-observation-collector branch February 7, 2020 02:17
@chriselion chriselion changed the title Develop observation collector Make CollectObservations take a VectorSensor Feb 13, 2020
@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.

2 participants