Skip to content

Fix/rand interval #563

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

Merged
merged 8 commits into from
Dec 20, 2021

Conversation

mo8it
Copy link
Contributor

@mo8it mo8it commented Dec 18, 2021

If at least one of the two endpoints are excluded from an interval, the rand function should not choose it.


Why the Union with Array{<:Interval}? I did remove it because it is similar with the functionality in Base:

julia> rand([ClosedInterval(i, i + 1) for i in 1:10])
7..8

julia> @which rand(fill(ClosedInterval(1.0, 2.0), (2,2)))
rand(X) in Random at /usr/share/julia/stdlib/v1.7/Random/src/Random.jl:259

julia> using ReinforcementLearning

julia> rand([ClosedInterval(i, i + 1) for i in 1:10])
5..6

julia> @which rand(fill(ClosedInterval(1.0, 2.0), (2,2)))
rand(s::Union{Array{<:Interval}, Interval}) in ReinforcementLearningEnvironments at /home/mo/.julia/packages/ReinforcementLearningEnvironments/UhI2A/src/base.jl:9

What was the intention of it? I guess, the intention is to get a random action from each interval, not a random interval. This was not working. If this functionality is needed, I can add a commit for it.


Also, I did add two methods to choose one of the endpoints if it is not an open interval. This has better performance than rand, but does not restrict to using IntervalSets, because we always have a fallback to rand.

@mo8it
Copy link
Contributor Author

mo8it commented Dec 18, 2021

The first commit is old, sorry. But it is merged anyway.

@findmyway
Copy link
Member

What was the intention of it? I guess, the intention is to get a random action from each interval, not a random interval. This was not working. If this functionality is needed, I can add a commit for it.

You guessed it. Since we now have Space as the container, we don't need that function any more.


Could you resolve the conflict first? So that we can get this merged into the master branch.

@mo8it
Copy link
Contributor Author

mo8it commented Dec 19, 2021

Is it fine now?

@findmyway findmyway merged commit 8c0a317 into JuliaReinforcementLearning:master Dec 20, 2021
@mo8it mo8it deleted the fix/rand_interval branch December 20, 2021 11:57
harwiltz pushed a commit to harwiltz/ReinforcementLearning.jl that referenced this pull request Mar 1, 2022
* Fix dummy action for continuous action spaces

* Fixed rand of an interval

* dummy action for (half-)closed intervals

* Update src/ReinforcementLearningEnvironments/src/base.jl

Co-authored-by: Jun Tian <find_my_way@foxmail.com>

* remove get_dummy_action for intervals

* Fix kind fo typo

Co-authored-by: Jun Tian <find_my_way@foxmail.com>
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.

2 participants