JuliaReinforcementLearning / GridWorlds.jl

Help! I'm lost in the flatland!
MIT License
47 stars 9 forks source link

`env[:, :, :] .= false` not working as expected #111

Closed Sid-Bhatia-0 closed 3 years ago

Sid-Bhatia-0 commented 3 years ago

Broadcasted setindex! for AbstractGridWorld objects, for example, like env[:, :, :] .= false, doesn't actually modify the env object's world field, even though we have used the forward macro here

If such behaviour is needed, use something like the following (for now) instead:

world = get_world(env)
world[:, :, :] .= false
findmyway commented 3 years ago

Have you read the doc about broadcast first? Let me know what you have tried if it still doesn't work as expected.

Sid-Bhatia-0 commented 3 years ago

This is what I tried

Base.broadcast!(f, env::AbstractGridWorld, args...) = broadcast!(f, get_world(env), args...)
Base.broadcast!(f, world::GridWorldBase, args...) = broadcast!(f, get_grid(world), args...)

Another thing I tried was to try to forward Base.view to AbstractGridWorld.world and GridWorldBase.grid. But these gave some index error.

The docs mention that

X[begin+1:end] .= sin.(Y), then it translates to broadcast! on a view, e.g. broadcast!(sin, view(X, firstindex(X)+1:lastindex(X)), Y)

But what does env[:, :, :] .= false translate to? There is no function. It is just assignment, which I thought would eventually call setindex!. But it doesn't work even on forwarding setindex!.

findmyway commented 3 years ago
  1. Define BroadcastStyle
  2. Define Base.copyto!

An example:

julia> struct X{A}
       x::A
       end

julia> Broadcast.BroadcastStyle(::Type{X{A}}) where A = Broadcast.BroadcastStyle(A)

julia> Base.copyto!(x::X, args...) = copyto!(x.x, args...)

julia> Base.size(x::X) = size(x.x)

julia> x = X([1,2,3])
X{Array{Int64,1}}([1, 2, 3])

julia> x .= 0
3-element Array{Int64,1}:
 0
 0
 0

julia> x
X{Array{Int64,1}}([0, 0, 0])
Sid-Bhatia-0 commented 3 years ago

Here X corresponds to AbstractGridWorld, right? AbstractGridWorld is not a parametric type. Here's what I tried (which didn't work):

julia> using GridWorlds

julia> using MacroTools

julia> Broadcast.BroadcastStyle(GridWorldBase)
Base.Broadcast.DefaultArrayStyle{3}()

julia> Broadcast.BroadcastStyle(::Type{<:AbstractGridWorld}) = Broadcast.BroadcastStyle(GridWorldBase)

julia> MacroTools.@forward AbstractGridWorld.world Base.size, Base.copyto!

julia> env = EmptyRoom()
Full View:
████████
█⋅⋅⋅⋅⋅⋅█
█⋅⋅⋅⋅⋅⋅█
█⋅⋅⋅⋅⋅⋅█
█♥⋅⋅⋅⋅⋅█
█⋅⋅⋅→⋅⋅█
█⋅⋅⋅⋅⋅⋅█
████████

Agent's View:
█⋅↓⋅⋅
█⋅⋅⋅⋅
█⋅⋅⋅⋅
█████
~~~~~

julia> env[:, :, :] .= false
3×8×8 BitArray{3}:
[:, :, 1] =
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0

[:, :, 2] =
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0

[:, :, 3] =
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0

[:, :, 4] =
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0

[:, :, 5] =
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0

[:, :, 6] =
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0

[:, :, 7] =
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0

[:, :, 8] =
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0
 0  0  0  0  0  0  0  0

julia> env
Full View:
████████
█⋅⋅⋅⋅⋅⋅█
█⋅⋅⋅⋅⋅⋅█
█⋅⋅⋅⋅⋅⋅█
█♥⋅⋅⋅⋅⋅█
█⋅⋅⋅→⋅⋅█
█⋅⋅⋅⋅⋅⋅█
████████

Agent's View:
█⋅↓⋅⋅
█⋅⋅⋅⋅
█⋅⋅⋅⋅
█████
~~~~~
findmyway commented 3 years ago

Here X corresponds to AbstractGridWorld, right? AbstractGridWorld is not a parametric type.

I just provide an example above to illustrate how to make it work.

Here's what I tried (which didn't work):

So could you explain what do you mean by didn't work?

You may need to dig into details to figure out why the MWE I provided above works but it doesn't work in your case.

Sid-Bhatia-0 commented 3 years ago

I just provide an example above to illustrate how to make it work. You may need to dig into details to figure out why the MWE I provided above works but it doesn't work in your case.

Ok. I'll see. I think this operation is returning a new array instead of modifying the underlying world.

So could you explain what do you mean by didn't work?

As you can see in the example I gave, the env[:, :, :] .= false returns a new array and doesn't modify the env in the end.

findmyway commented 3 years ago

As you can see in the example I gave, the env[:, :, :] .= false returns a new array and doesn't modify the env in the end.

I think that's because the BitArray is wrapped in the GridWorldBase.

By the way, I don't think env[:, :, :] .= false is a good practice to modify the inner world representation. It violates our assumption to separate the representation layer and logic layer.

Sid-Bhatia-0 commented 3 years ago

I think that's because the BitArray is wrapped in the GridWorldBase.

Maybe. I'm not really sure.

By the way, I don't think env[:, :, :] .= false is a good practice to modify the inner world representation. It violates our assumption to separate the representation layer and logic layer.

Yes. In fact, I was contemplating if I should remove indexing env completely since we can already get the underlying world and modify it directly.

the representation layer and logic layer.

I don't understand these terms clearly. Can you please clarify?

findmyway commented 3 years ago

Here I mean we only need to expose the env(action) interface to modify inner state (this is what I mean the logic layer). And provide different views of the game via state(env, state_style) and do not modify them directly (this is what I mean the logic layer).

Sid-Bhatia-0 commented 3 years ago

Here I mean we only need to expose the env(action) interface to modify inner state (this is what I mean the logic layer). And provide different views of the game via state(env, state_style) and do not modify them directly (this is what I mean the logic layer).

I see. Is it fine to create methods internal to GridWorlds.jl like get_agent(env), set_world!(env, world) etc... Technically set_world!(env, world) does modify the game state. But these methods are useful internally to GridWorlds during creating environments, say for use inside reset!(env). Should methods like set_world! not be exported?

findmyway commented 3 years ago

Should methods like set_world! not be exported?

I prefer to keep it private. We can export it later when we really need it as a user.

Sid-Bhatia-0 commented 3 years ago

Fixed in #113