Fix naming consistency and add missing hook tests (#1049)

* Make nsteps naming consistent

* add missing hook tests

* basic hook tests and nesting fixes

---------

Co-authored-by: Jeremiah Lewis <--get>

Author: jeremiahpslewis

docs/homepage/guide/index.md

@@ -84,7 +84,7 @@ Usually a closure or a functional object will be used to store some intermediate
 
 In most cases, you don't need to write a customized hook. Some generic hooks are provided so that you can inject logic at the appropriate time:
 
-- [`DoEveryNStep`](https://juliareinforcementlearning.org/ReinforcementLearning.jl/latest/rl_core/#ReinforcementLearningCore.DoEveryNStep)
+- [`DoEveryNSteps`](https://juliareinforcementlearning.org/ReinforcementLearning.jl/latest/rl_core/#ReinforcementLearningCore.DoEveryNSteps)
 - [`DoEveryNEpisode`](https://juliareinforcementlearning.org/ReinforcementLearning.jl/latest/rl_core/#ReinforcementLearningCore.DoEveryNEpisode)
 
 However, if you do need to write a customized hook, the following methods must be provided:
@@ -98,10 +98,10 @@ If your hook is a subtype of `AbstractHook`, then all the above methods will hav
 
 ## How to use TensorBoard?
 
-This package adopts a non-invasive way for logging. So you can log everything you like with a hook. For example, to log the loss of each step. You can use the [`DoEveryNStep`](https://juliareinforcementlearning.org/ReinforcementLearning.jl/latest/rl_core/#ReinforcementLearningCore.DoEveryNStep).
+This package adopts a non-invasive way for logging. So you can log everything you like with a hook. For example, to log the loss of each step. You can use the [`DoEveryNSteps`](https://juliareinforcementlearning.org/ReinforcementLearning.jl/latest/rl_core/#ReinforcementLearningCore.DoEveryNSteps).
 
 ```julia
-DoEveryNStep() do t, agent, env
+DoEveryNSteps() do t, agent, env
     with_logger(lg) do
         @info "training" loss = agent.policy.learner.loss
     end
@@ -117,7 +117,7 @@ run(
     agent,
     env,
     stop_condition,
-    DoEveryNStep(EVALUATION_FREQ) do t, agent, env
+    DoEveryNSteps(EVALUATION_FREQ) do t, agent, env
         run(agent, env, eval_stop_condition, eval_hook)
     end
 )

docs/src/How_to_use_hooks.md

@@ -85,7 +85,7 @@ Sometimes, we'd like to periodically run some functions. Two handy hooks are
 provided for this kind of tasks:
 
 - [`DoEveryNEpisode`](@ref)
-- [`DoEveryNStep`](@ref)
+- [`DoEveryNSteps`](@ref)
 
 Following are some typical usages.
 
@@ -160,7 +160,7 @@ run(
     policy,
     env,
     StopAfterNSteps(10_000),
-    DoEveryNStep(n=1_000) do t, p, e
+    DoEveryNSteps(n=1_000) do t, p, e
         ps = params(p)
         f = joinpath(parameters_dir, "parameters_at_step_$t.bson")
         BSON.@save f ps

src/ReinforcementLearningCore/src/core/hooks.jl

@@ -7,7 +7,7 @@ export AbstractHook,
     BatchStepsPerEpisode,
     TimePerStep,
     DoEveryNEpisode,
-    DoEveryNStep,
+    DoEveryNSteps,
     DoOnExit
 
 using UnicodePlots: lineplot, lineplot!
@@ -38,10 +38,10 @@ struct ComposedHook{T<:Tuple} <: AbstractHook
     ComposedHook(hooks...) = new{typeof(hooks)}(hooks)
 end
 
-Base.:(+)(h1::AbstractHook, h2::AbstractHook) = ComposedHook((h1, h2))
-Base.:(+)(h1::ComposedHook, h2::AbstractHook) = ComposedHook((h1.hooks..., h2))
-Base.:(+)(h1::AbstractHook, h2::ComposedHook) = ComposedHook((h1, h2.hooks...))
-Base.:(+)(h1::ComposedHook, h2::ComposedHook) = ComposedHook((h1.hooks..., h2.hooks...))
+Base.:(+)(h1::AbstractHook, h2::AbstractHook) = ComposedHook(h1, h2)
+Base.:(+)(h1::ComposedHook, h2::AbstractHook) = ComposedHook(h1.hooks..., h2)
+Base.:(+)(h1::AbstractHook, h2::ComposedHook) = ComposedHook(h1, h2.hooks...)
+Base.:(+)(h1::ComposedHook, h2::ComposedHook) = ComposedHook(h1.hooks..., h2.hooks...)
 
 @inline function _push!(stage::AbstractStage, policy::P, env::E, hook::H, hook_tuple...) where {P <: AbstractPolicy, E <: AbstractEnv, H <: AbstractHook}
     Base.push!(hook, stage, policy, env)
@@ -286,26 +286,22 @@ function Base.push!(hook::TimePerStep, ::PostActStage, agent, env)
 end
 
 """
-    DoEveryNStep(f; n=1, t=0)
+    DoEveryNSteps(f; n=1, t=0)
 
 Execute `f(t, agent, env)` every `n` step.
 `t` is a counter of steps.
 """
-mutable struct DoEveryNStep{F,T} <: AbstractHook where {F,T<:Integer}
+mutable struct DoEveryNSteps{F} <: AbstractHook where {F}
     f::F
-    n::T
-    t::T
-
-    function DoEveryNStep(f; n=1, t=0)
-        new{typeof(f),Int64}(f, n, t)
-    end
-
-    function DoEveryNStep{T}(f; n=1, t=0) where {T<:Integer}
-        new{typeof(f),T}(f, n, t)
+    n::Int
+    t::Int
+    
+    function DoEveryNSteps(f::F; n=1, t=0) where {F}
+        new{F}(f, n, t)
     end
 end
 
-function Base.push!(hook::DoEveryNStep, ::PostActStage, agent, env)
+function Base.push!(hook::DoEveryNSteps, ::PostActStage, agent, env)
     hook.t += 1
     if hook.t % hook.n == 0
         hook.f(hook.t, agent, env)

src/ReinforcementLearningCore/test/core/hooks.jl

@@ -1,3 +1,5 @@
+struct MockHook <: AbstractHook end 
+
 """
 test_noop!(hook; stages=[PreActStage()])
 
@@ -36,6 +38,41 @@ function test_run!(hook::AbstractHook)
     return hook_
 end
 
+@testset "AbstractHook + AbstractHook" begin
+    @test MockHook() + MockHook() == ComposedHook(MockHook(), MockHook())
+end
+
+@testset "ComposedHook + AbstractHook" begin
+    struct MockHook <: AbstractHook end 
+    @test ComposedHook(MockHook()) + MockHook() == ComposedHook(MockHook(), MockHook())
+end
+
+@testset "AbstractHook + ComposedHook" begin
+    @test MockHook() + ComposedHook(MockHook()) == ComposedHook(MockHook(), MockHook())
+end
+
+@testset "ComposedHook + ComposedHook" begin
+    @test ComposedHook(MockHook()) + ComposedHook(MockHook()) == ComposedHook(MockHook(), MockHook())
+end
+
+@testset "push! method for ComposedHook" begin
+    stage = PreActStage()
+    policy = RandomPolicy()
+    env = TicTacToeEnv()
+    composed_hook = ComposedHook(MockHook(), MockHook())
+    push!(composed_hook, stage, policy, env)
+    @test composed_hook.hooks == (MockHook(), MockHook())
+end
+
+@testset "push! method for ComposedHook with multiple hooks" begin
+    stage = PreActStage()
+    policy = RandomPolicy()
+    env = TicTacToeEnv()
+    composed_hook = ComposedHook(MockHook(), MockHook())
+    push!(composed_hook, stage, policy, env)
+    @test composed_hook.hooks == (MockHook(), MockHook())
+end
+
 @testset "TotalRewardPerEpisode" begin
     h_1 = TotalRewardPerEpisode(; is_display_on_exit=true)
     h_2 = TotalRewardPerEpisode(; is_display_on_exit=false)
@@ -57,9 +94,9 @@ end
     end
 end
 
-@testset "DoEveryNStep" begin
-    h_1 = DoEveryNStep((hook, agent, env) -> (env.pos += 1); n=2)
-    h_2 = DoEveryNStep((hook, agent, env) -> (env.pos += 1); n=1)
+@testset "DoEveryNSteps" begin
+    h_1 = DoEveryNSteps((hook, agent, env) -> (env.pos += 1); n=2)
+    h_2 = DoEveryNSteps((hook, agent, env) -> (env.pos += 1); n=1)
 
     for h in (h_1, h_2)
         env = RandomWalk1D()