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

[WIP] Observation Types #4825

Merged
merged 25 commits into from
Jan 13, 2021
Merged

[WIP] Observation Types #4825

merged 25 commits into from
Jan 13, 2021

Conversation

awjuliani
Copy link
Contributor

@awjuliani awjuliani commented Jan 5, 2021

Proposed change(s)

Adds a ObservationType field to the ObservationSpec.

ObservationType is an enum consisting of one of the following:

  • Default
  • Goal
  • Reward
  • Message

Todo:

  • Mark new C# interface as internal.
  • Ensure all current observation sensors are marked as observations.
  • Modify all tests.

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



# This class is part of an EXPERIMENTAL API.
class UnityToExternalProto(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you expect this file to change (especially with this new class)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an artifact of my building the proto files using a newer version of grpc-tools, as I am still unable to install older versions. I'll try again to figure out the problem and build these with the version we are using internally though, since there are obviously discrepancies in what is being produced.

@chriselion
Copy link
Contributor

General feedback:

  • Should this be called ObservationType instead of SensorType?
  • Is there a less generic term that we can use than "(Sensor/Observation)Type"? That would also get around using type as a member variable on the python side, which is OK but discouraged since it's a builtin function.

@awjuliani
Copy link
Contributor Author

General feedback:

  • Should this be called ObservationType instead of SensorType?
  • Is there a less generic term that we can use than "(Sensor/Observation)Type"? That would also get around using type as a member variable on the python side, which is OK but discouraged since it's a builtin function.

Thanks for the code review, Chris. These are both good questions. The issue with ObservationType is that one of those types is "Observation," so we'd need to change whatever that is called to disambiguate it. Vince actually renamed the ObservationSpec to a SensorSpec, so this naming for the "type" lines us.

I agree though that "type" is too generic, and potentially problematic wrt python naming. Definitely open to other more appropriate names for it.

@awjuliani awjuliani changed the title [WIP] Sensor Types [WIP] Observation Types Jan 7, 2021
/// <summary>
/// Sensor interface for sensors with variable types.
/// </summary>
internal interface ITypedSensor
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this interface be public? Are users expected to add ObservationType to their sensors or not for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't need to be, since any ISensor that isn't an ITypedSensor should be treated as Default type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intention is similar to making the DimensionProperty internal for now. To not change the public API until there are actual features to use it.

ml-agents-envs/mlagents_envs/base_env.py Show resolved Hide resolved
@awjuliani awjuliani merged commit cb8e220 into master Jan 13, 2021
@delete-merged-branch delete-merged-branch bot deleted the sensor-types branch January 13, 2021 18:11
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 13, 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.

3 participants