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

Refactor to allow containers to be directly used as spaces #9

Merged
merged 14 commits into from
Aug 5, 2022

Conversation

zsunberg
Copy link
Member

@findmyway Here is my start to a PR related to #8 . I haven't started implementing anything yet, just wrote down my ideas in the README. Let me know what you think.

@mossr, @mykelk, @jamgochiana, feel free to chime in if you have any comments.

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2022

Codecov Report

Merging #9 (9ee91b5) into main (ddfd7ab) will increase coverage by 28.76%.
The diff coverage is 86.90%.

@@             Coverage Diff             @@
##             main       #9       +/-   ##
===========================================
+ Coverage   58.13%   86.90%   +28.76%     
===========================================
  Files           1        3        +2     
  Lines          43       84       +41     
===========================================
+ Hits           25       73       +48     
+ Misses         18       11        -7     
Impacted Files Coverage Δ
src/product.jl 74.19% <74.19%> (ø)
src/basic.jl 82.35% <82.35%> (+24.21%) ⬆️
src/array.jl 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

README.md Outdated
Continuous spaces represent sets that have an uncountable number of elements they have a `SpaceStyle` of type `ContinuousSpaceStyle`. CommonRLSpaces does not adopt a rigorous mathematical definition of a continuous set, but, roughly, elements in the interior of a continuous space have other elements arbitrarily close to them.
Continuous spaces have two additional interface functions:
- `bounds(space)` returns upper and lower bounds in a tuple. For example, if `space` is a unit circle, `bounds(space)` will return `([-1.0, -1.0], [1.0, 1.0])`. This allows agents to choose policies that appropriately cover the space e.g. a normal distribution with a mean of `mean(bounds(space))` and a standard deviation of half the distance between the bounds.
- [I have a few other ideas, but we can discuss later]
Copy link
Member

Choose a reason for hiding this comment

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

Another more common function is clamp


julia> size(s)
julia> elsize(s)
Copy link
Member

Choose a reason for hiding this comment

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

we may also have eltype implemented

README.md Outdated

### Hybrid spaces

[need to figure this out]
Copy link
Member

Choose a reason for hiding this comment

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

For Hybrid Spaces, I think we can still use product(s1, s2) to create a hybrid space.

To help access each sub space easily, s1 and s2 may be named spaces and the hybrid space need to support getindex.

@zsunberg
Copy link
Member Author

Ok, I have now added Box. A few things that I would like feedback on:

  1. I removed the size parameter from the space styles because this seems like a trait that is orthogonal to continuity, and it only really fully makes sense for array spaces.
  2. By default, Box uses StaticArrays even when constructed with builtin arrays.

@findmyway
Copy link
Member

Looking at the definition. The Box seems to be a specialized case of a HybridSpace.

I mean it's more or less the same with the result of product(intervals...), except that it provides two extra information on the element:

  1. Its element is an Array, which also means we know the eltype and size
  2. The bounds are preallocated

Such information are usually environment specific. Some may want a tuple instead of array. Or others do not care about the preallocation feature.

This makes me wonder that, maybe we can just define some traits to recognize some common elemental spaces. We don't need to care too much about the container type (the result type of product). It may be an Array, a Tuple, a NamedTuple, a Dict or whatever. To sample an element from the hybrid space, we simply create a similar container, except that each subspace is replaced with an inner sampling. So that we can still keep the name, type or size info from the container.

In some extreme cases, we can still add a dedicated implementation like Box above if we need the preallocation feature.

With the traits defiend above, I think this issue can be addressed JuliaMath/IntervalSets.jl#66

Note that we may need our customized definition of rand, in, eltype and elsize here to avoid type piracy.

rand(x) = rand(rng, x)
rand(rng, x) = rand(rng, SpaceStyle(x), x)

rand(rng, ::DiscreteSpace, x) = Random.rand(rng, x)
rand(rng, ::ContinuousSpace, x) = ...

rand(rng, ::Any, xs) = map(x -> rand(rng, x), xs)

This means product and HygirdSpace are replaced with more concrete constructors, like tuple and Tuple{DiscreteSpace, ContinuousSpace}.

src/product.jl Outdated Show resolved Hide resolved
@zsunberg zsunberg changed the title Refactor to allow containers to be directly used as spaces [Draft] Refactor to allow containers to be directly used as spaces Aug 2, 2022
@zsunberg zsunberg marked this pull request as draft August 2, 2022 01:43
@zsunberg
Copy link
Member Author

zsunberg commented Aug 2, 2022

@findmyway , thanks for the comments! This was very helpful in my thinking about product. I agree that we should not care too much about the type that product returns.

Do you agree with this statement that I just added to the README:

The Cartesian product of two spaces a and b can be constructed with c = product(a, b).

The exact form of the resulting space is unspecified and should be considered an implementation detail. The only guarantees are (1) that there will be one unique element of c for every combination of one object from a and one object from b and (2) that the resulting collection conforms to the interface above.

The TupleProductSpace constructor provides a specialized Cartesian product where each element is a tuple, i.e. TupleProductSpace(a, b) has elements of type Tuple{eltype(a), eltype(b)}.

That is my guiding principle for implementing product. I'll try to finish more of the implementation as soon as I get a chance, but it is not finished yet.

The Box seems to be a specialized case of a HybridSpace.

By the way, I was thinking that "hybrid" in HybridSpaceStyle should refer to whether the space is continuous or discrete, i.e. in addition to FiniteSpaceStyle and ContinuousSpaceStyle, we would also have HybridSpaceStyle. So not every product of two spaces should be a HybridSpaceStyle, and in particular Box is always ContinuousSpaceStyle (see the docstring I added for Box for clarification about what it is).

@findmyway
Copy link
Member

Yes, I agree.

And thanks for the explanation of HybridSpaceStyle. Now I have a better understanding of hybrid here.

@zsunberg
Copy link
Member Author

zsunberg commented Aug 4, 2022

Ok, I think this is almost ready to merge, except for a few docstrings and updating the readme. I should be able to add those tomorrow or so. I am very satisfied with how these basics turned out, so I hope everyone agrees that it is a good starting point.

It is currently missing any interface for hybrid continuous-discrete spaces, but we can add that in another chunk. There are also some other issues that I will file later (the two broken tests have raised some questions).

@zsunberg zsunberg changed the title [Draft] Refactor to allow containers to be directly used as spaces Refactor to allow containers to be directly used as spaces Aug 5, 2022
@zsunberg zsunberg marked this pull request as ready for review August 5, 2022 05:47
@zsunberg
Copy link
Member Author

zsunberg commented Aug 5, 2022

OK, I think this is ready to merge. @findmyway feel free to merge, or I will do it tomorrow if no one objects. I'll add issues tomorrow outlining the things that aren't done yet.

@findmyway findmyway merged commit 00e365d into main Aug 5, 2022
@zsunberg zsunberg mentioned this pull request Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants