Skip to content
This repository has been archived by the owner on May 6, 2021. It is now read-only.

Don't hardcode array type in MultiContinuousSpace. #28

Closed
wants to merge 1 commit into from

Conversation

aterenin
Copy link
Contributor

Right now this is hardcoded to Array{Float64}, which prevents users using, for example, CuArray{Float32} instead given an appropriate environment.

@aterenin
Copy link
Contributor Author

Huh, where did that CI failure come from? Tests all passed on my system before I pushed - strange. Will fix this, provided people want the change in this PR.

Copy link
Contributor

@jbrea jbrea left a comment

Choose a reason for hiding this comment

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

I like this idea.

)) || throw(ArgumentError("each element of $low must be less than $high"))
new{size(low),ndims(low)}(low, high)
end
struct MultiContinuousSpace{S, N, Fl<:AbstractFloat, AFl<:AbstractArray{Fl,N}} <: AbstractDiscreteSpace
Copy link
Contributor

@jbrea jbrea Jan 27, 2020

Choose a reason for hiding this comment

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

Does anything speak against MultiContinuousSpace{S, N, A <: AbstractArray{<:Number, N}}?


Base.eltype(::MultiContinuousSpace{S,N}) where {S,N} = Array{Float64,N}
Base.in(xs, s::MultiContinuousSpace{S,N}) where {S,N} =
Base.eltype(::MultiContinuousSpace{S,N,Fl}) where {S,Fl,N} = Array{Fl,N}
Copy link
Contributor

@jbrea jbrea Jan 27, 2020

Choose a reason for hiding this comment

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

I guess this should then become Base.eltype(::MultiContinuousSpace{S,N,AF1}) where {S,N,A} = A.

Base.eltype(::MultiContinuousSpace{S,N}) where {S,N} = Array{Float64,N}
Base.in(xs, s::MultiContinuousSpace{S,N}) where {S,N} =
Base.eltype(::MultiContinuousSpace{S,N,Fl}) where {S,Fl,N} = Array{Fl,N}
Base.in(xs, s::MultiContinuousSpace) =
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work without Base.in(xs, s::MultiContinuousSpace{S}) where S =

@findmyway
Copy link
Member

The idea sounds great. I'll integrate this feature in the JuliaReinforcementLearning/ReinforcementLearningCore.jl#1


Due to the heavy Wuhan_coronavirus_outbreak , I have plenty of time to work on this project in the next several weeks. Hope we can move forward and make a release soon.

@findmyway
Copy link
Member

FYI:

JuliaStats/Distributions.jl#960

@aterenin
Copy link
Contributor Author

aterenin commented Feb 1, 2020

Sorry for the delay with this - ICML deadline has been squeezing my time. I can get to this PR as soon as that deadline passes, if still needed by then!

@aterenin
Copy link
Contributor Author

It seems this has since been rewritten and moved into ReinforcementLearningCore.

@aterenin aterenin closed this Feb 19, 2020
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.

None yet

3 participants