Stop transformation steps when no longer possible and add verbose flag

[RenatoGeh]

Hi,

When having a high enough maxiter on learn_circuit (or init_maxiter on learn_strudel), Juice spits out the following error.

ERROR: ArgumentError: collection must be non-empty
Stacktrace:
 [1] _findmax(::Array{Any,1}, ::Colon) at ./array.jl:2197
 [2] #findmax#654 at ./reducedim.jl:887 [inlined]
 [3] findmax(::Array{Any,1}) at ./reducedim.jl:887
 [4] eFlow(::Array{UInt64,2}, ::Array{UInt64,2}, ::Array{Tuple{Node,Node},1}) at /home/renatogeh/.julia/packages/ProbabilisticCircuits/Uh1Ay/src/structurelearner/heuristics.jl:11
 [5] heuristic_loss(::StructSumNode, ::DataFrame; pick_edge::String, pick_var::String) at /home/renatogeh/.julia/packages/ProbabilisticCircuits/Uh1Ay/src/structurelearner/heuristics.jl:53
 [6] loss at /home/renatogeh/.julia/packages/ProbabilisticCircuits/Uh1Ay/src/structurelearner/learner.jl:40 [inlined]
 [7] split_step(::StructSumNode; loss::ProbabilisticCircuits.var"#loss#343"{String,String,Nothing,DataFrame}, depth::Int64, sanity_check::Bool) at /home/renatogeh/.julia/packages/LogicCircuits/Z9ob4/src/transformations.jl:347
 [8] (::ProbabilisticCircuits.var"#pc_split_step#344"{Int64,Float64,Bool,Int64,Bool,Float64,DataFrame,ProbabilisticCircuits.var"#loss#343"{String,String,Nothing,DataFrame}})(::StructSumNode) at /home/renatogeh/.julia/packages/ProbabilisticCircuits/Uh1Ay/src/structurelearner/learner.jl:44
 [9] struct_learn(::StructSumNode; primitives::Array{ProbabilisticCircuits.var"#pc_split_step#344"{Int64,Float64,Bool,Int64,Bool,Float64,DataFrame,ProbabilisticCircuits.var"#loss#343"{String,String,Nothing,DataFrame}},1}, kwargs::Dict{ProbabilisticCircuits.var"#pc_split_step#344"{Int64,Float64,Bool,Int64,Bool,Float64,DataFrame,ProbabilisticCircuits.var"#loss#343"{String,String,Nothing,DataFrame}},Tuple{}}, maxiter::Int64, stop::ProbabilisticCircuits.var"#log_per_iter#345"{Int64,Int64,Bool,DataFrame}) at /home/renatogeh/.julia/packages/LogicCircuits/Z9ob4/src/transformations.jl:362
 [10] learn_circuit(::DataFrame, ::StructSumNode, ::PlainVtreeInnerNode; pick_edge::String, pick_var::String, depth::Int64, pseudocount::Float64, sanity_check::Bool, maxiter::Int64, seed::Nothing, return_vtree::Bool, batch_size::Int64, splitting_data::Nothing, use_gpu::Bool, entropy_reg::Float64) at /home/renatogeh/.julia/packages/ProbabilisticCircuits/Uh1Ay/src/structurelearner/learner.jl:65
 [11] learn_circuit(::DataFrame; pick_edge::String, pick_var::String, depth::Int64, pseudocount::Float64, entropy_reg::Float64, sanity_check::Bool, maxiter::Int64, seed::Nothing, return_vtree::Bool) at /home/renatogeh/.julia/packages/ProbabilisticCircuits/Uh1Ay/src/structurelearner/learner.jl:20
 [12] top-level scope at REPL[31]:1
 [13] run_repl(::REPL.AbstractREPL, ::Any) at /build/julia/src/julia-1.5.3/usr/share/julia/stdlib/v1.5/REPL/src/REPL.jl:288

This is because there are no more possible edges to apply the split operation on. Instead of throwing out an error, Juice should probably stop the split iteration gracefully and skip to the next step of learning.

This PR addresses this issue and early stops when no longer possible to apply any transformation steps. It also adds a verbose flag to learn_circuit and learn_strudel to suppress the printing when not needed.

Here's a minimal reproducible example for the above stacktrace:

using ProbabilisticCircuits, DataFrames
D = DataFrame(convert(BitArray, rand(Bool, 100, 4)))
pc = learn_circuit(D; maxiter = 50)

Thanks

[MhDang]

hi,

Thanks for the report.

Could you please add the reproducible examples as a test, so we can make sure to fix this bug?

When there are no more edges to split on, the algorithm should finish and return the last circuit. Returning nothing may lose the last circuit.

One possible solution could be:
[1] In learner.jl line 64, return a false flag and the old circuit;
[2] In line 69, for function struct_learn, the kwarg stop should get both the flag and the circuit as parameters and return true if the algorithm should stop.
We may also need to change several other places to uniform the arguments API.

[RenatoGeh]

Oops. I only noticed now that one of my changes was on LogicCircuits and it did exactly what you're referring to, but since the repos are separate I didn't even notice during the commit process. The changes in LogicCircuits are minimal and are as follows:

diff --git a/src/transformations.jl b/src/transformations.jl
index 9097b31..01a7d95 100644
--- a/src/transformations.jl
+++ b/src/transformations.jl
@@ -344,6 +344,8 @@ end
 Split step
 """
 function split_step(circuit::Node; loss=random_split, depth=0, sanity_check=true)
+    res = loss(circuit)
+    if isnothing(res) return nothing end
     edge, var = loss(circuit)
     split(circuit, edge, var; depth=depth, sanity_check=sanity_check)
 end
@@ -354,12 +356,17 @@ Structure learning manager
 function struct_learn(circuit::Node; 
     primitives=[split_step], 
     kwargs=Dict(split_step=>(loss=random_split, depth=0)),
-    maxiter=typemax(Int), stop::Function=x->false)
+    maxiter=typemax(Int), stop::Function=x->false, verbose = true)
 
     for iter in 1 : maxiter
         primitive_step = rand(primitives)
         kwarg = kwargs[primitive_step]
-        c2, _ = primitive_step(circuit; kwarg...)
+        r = primitive_step(circuit; kwarg...)
+        if isnothing(r)
+            verbose && println("No more edges to transform. Skipping next iterations.")
+            return circuit
+        end
+        c2, _ = r
         if stop(c2)
             return c2
         end

Your solution looks cleaner though. I like the idea of using stop.

[RenatoGeh]

Going back to this PR I noticed we have a couple of problems with this approach (of using stop for stopping - that's a weird sentence).

1. stop comes after primitive_step, meaning that we still have to somehow handle the case when there are no more candidates for primitive_step even before stop runs;
2. Suppose we have split and clone as primitives. If split has no more candidates to transform, the user might still want clone to run, so we should keep track of how many primitives do not have candidates and skip these.

For (1) a possible direction we could take (which is what I do in the post directly above this one) is require transformations to return a nothing in case no candidates are available. We can then track which primitives should be skipped within struct_learn (which wouldn't be possible - without some convoluted code - within stop) for solving (2).

What do you think?

[khosravipasha]

@RenatoGeh Interesting, yeah this makes sense and looks good to me.

One thing not sure at is if clone without split makes sense or not.
@MhDang What do you think? You have more experience in dealing with split and clones.

[MhDang]

@RenatoGeh @khosravipasha

This solution looks good to me as well.

Clone without split is valid on the circuit as long as the or node to be cloned has more than one parent. The clone operation makes a copy of this or node and redirects some of the parents to the copy.

[RenatoGeh]

Great. I pushed the necessary changes, and now the PR is ready for review. I also pushed the LogicCircuits side of things here Tractables/LogicCircuits.jl#68.

[khosravipasha]

Nice, thanks, I guess we will push the LogicCircuit ones first and then rerun the tests here and should be good to go.