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

Add contains/2 to Gyx.Core.Spaces #40

Merged
merged 13 commits into from
Apr 8, 2019
Merged

Add contains/2 to Gyx.Core.Spaces #40

merged 13 commits into from
Apr 8, 2019

Conversation

doctorcorral
Copy link
Owner

No description provided.

@doctorcorral doctorcorral added the enhancement New feature or request label Mar 27, 2019
@doctorcorral doctorcorral self-assigned this Mar 27, 2019
@doctorcorral
Copy link
Owner Author

Implementation for Gyx.Core.Spaces.Box is still missing

Copy link
Collaborator

@davoclavo davoclavo left a comment

Choose a reason for hiding this comment

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

Looks good! I just left two minor change suggestions

alias Gyx.Core.Spaces.{Discrete, Box}

@type space :: Discrete.t() | Box.t()
@type discete_point :: integer()
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor typo: discete_point -> discrete_point

Copy link
Owner Author

Choose a reason for hiding this comment

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

Fixed

@@ -0,0 +1,16 @@
defmodule Gyx.Core.Spaces.Tuple do
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about something like Gyx.Core.Spaces.List or Vector? to imply that any number of items is possible

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think these are important decisions. How to name such module, and what data structure use for such implementation.

The motivation for using Tuple as naming is that it follows the convention on Gym.
In fact, it represents a fixed size, fixed position structure. Any space is always fixed for a particular environment (its action and observation spaces do not change), but they can differ widely among different environments.

The motivation for NOT using an actual tuple as data structure, is that it is not possible to type spec the elements to be a space :: Gyx.Core.Spaces.Box | Gyx.Core.Spaces.Discrete
And it might be unnatural to construct a tuple structure from parsing a Gym Tuple __repr__ (see Gyx.Gym.Utils where this case must be handled).

That's why I vote to name it Tuple, having the key spaces holding a list.

@doctorcorral doctorcorral merged commit 54f2f86 into master Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants