Random decision lists and ripper - Trombini's work

[Perro2110]

I'm opening this PR for Trombini's work so that it will be easier to review the code and then merge it.

[edo-007]

I'm not sure if the rule growth criterion for RIPPER is the same as the one implemented in findbestantecedent.
To be checked.
According to the paper this function should be called "replace_rule"

[NickT42]

As for the call to findbestantecedent, I based myself on the following passage from the original RIPPER paper

For each rule Ri, two alternative rules are constructed. The replacement for Ri is formed by growing and then pruning a rule Ri', where pruning is guided so as to minimize the error of the entire ruleset R1, ..., Ri', ..., Rk on the pruning data. The revision of Ri is formed analogously, except that the revision is grown by greedily adding conditions to Ri, rather than the empty rule.

Then, as for which rule to keep, the paper writes

Finally a decision is made as to whether the finalnal theory should include the revised rule, the replacement
rule, or the original rule. This decision is made using the MDL heuristic.

I think it's implied that the process of creating a rule and/or expanding an existing one is the same as IREP*, which is described as follows

GrowRule repeatedly adds the condition that maximizes FOIL's information gain criterion until the rule covers no negative examples from the growing dataset.

This seems like what findbestantecedent was already doing, with the only difference being in the possibilities it allows in its parameters.

[edo-007]

From what I’ve gathered from the literature, CN2 is the 'cautious' one. During rule construction, it evaluates whether adding a new condition is actually useful or if it’s just chasing noise in the data (pre-pruning). If the test shows it’s not worth it, it stops immediately.

RIPPER, on the other hand, is the 'overachiever.' During the growth phase, it goes full speed ahead, specializing the rule until it’s 'perfect'—meaning it doesn't make a single error on the training data—without worrying that it might be becoming too specific or complex. Only later, and with more care, does it use a separate validation set to 'prune' away the unnecessary branches and simplify the rule.

It seems clear to me, then, that the stopping criterion for rule growth must be different and managed according to the specific case, or, at least, make sure that in the case of RIPPER the rules generated in the grow phase actually overfit the data.

[NickT42]

I agree with what you said.
On an implementation level, findbestantecedent seems to be using the greedy/"overachiever" RIPPER approach: it keeps specializing a rule until doing so would result in a coverage smaller than min_rule_coverage.
When that happens, the function sortedantecedents returns nothing, and the loop in findbestantecedent halts.

If other the "cautious" CN2-like type of rule finding is to be implemented, we could consider adding the possibility to pass a stopping condition as an optional user-definable parameter.

[edo-007]

I agree with what you said.
However, I'd say that in CN2 this hyperspecialization depends on the heuristic you want to use (e.g., Laplace accuracy, unlike entropy, doesn't always favor massively specific rules).
Do you agree ?

[NickT42]

The method for finding an antecedent in CN2 can still be implemented using the current findbestantecedent function.
The pseudocode for finding a rule that I'm referring to is the one in Find_BestConditionExpression in https://en.wikipedia.org/wiki/CN2_algorithm.
The method still has a beam of antecedents that are incrementally specialized, and the main loop stops when ConditionalExpressionSet is empty, which can only happen if TrialConditionalExpressionSet is empty, or in other words, if no further specializations are possible, just like in findbestantecedent.
The only "difference" with the current findbestantecedent function is that before selecting a new "best" test, there is a test for statistical significance as well as a scoring.
This could easily be implemented given the already existing framework, by simply returning an infinite loss value if the rule does not pass a test for statistical significance. In that case, a rule that does not satisfy the test could never be chosen to be returned.

Likewise, for any other type of "test" aimed to select or discard a rule, one could simply use the already-existing loss function system, and return Inf when the test is not passed, and that makes sure the rule must be discarded.
This, coupled with a custom stopping condition, seems like it should be enough to handle every case I've seen so far.

Overall I feel like the general structure for both RIPPER and CN2 is the same: incremental specialization of a beam of antecedents, using some kind of metric/test/loss to determine which one is best, and a predetermined stopping condition, which in both cases is if no further specializations are possible, but which can very well be implemented to be (optionally) defined in Sole.jl.

Trying to "force" certain combinations of stopping conditions or tests seems complicated, and I don't know how it would be useful.

[edo-007]

The logic for tracking which samples are covered by the current ruleset is duplicated across irepstar, ripperk, and merge_ruleset_satmasks. A single abstraction responsible for maintaining and updating the coverage mask would eliminate the duplication and make the training loops easier to read.

[edo-007]

Overall the code looks good.
I would say it is algorithmically faithful to the original implementation( I left a comment about this).
The main thing worth working on is cleanliness — adding a few layers of abstraction in the right places would make it significantly easier to read and maintain. Example: wrapping the parallel training state variables into a struct, cleaning up how algorithm parameters are passed around, and consolidating the duplicated coverage tracking logic.

[PasoStudio73]

Be careful on dependencies: are freaking outdated!
I suggest you to open project.toml and update dependencies:

[compat]
BenchmarkTools = "1"
CSV = "0.10"
CategoricalArrays = "1"
DataFrames = "1.8"
DecisionTree = "0.12"
Distributions = "0.25"
FillArrays = "1"
Logging = "1.11"
MLJ = "0.21 - 0.23"
MLJBase = "1.12"
MLJDecisionTreeInterface = "0.4"
MLJModelInterface = "1.12"
MLJModels = "0.18"
Parameters = "0.12"
Printf = "1.11 - 1.12"
RDatasets = "0.8"
Reexport = "1"
Revise = "3.14"
SoleBase = "0.13"
SoleData = "0.16"
SoleLogics = "0.13"
SoleModels = "0.10"
StatsBase = "0.34"
Tables = "1.12"
Test = "1"
julia = "1"

And remember that packages such as BenchmarkTools, Revise and Test should never been placed into dependencies: those packages are intended for developement only!

Now that I've updated deps in my local repo, I'm going to test this package, but at a first glance, looks awesome!

[PasoStudio73]

I'm playing with tests and with every test I ended up with this error:

ERROR: TaskFailedException

    nested task error: MethodError: no method matching _multivariate_scalar_alphabet(::Vector{…}, ::Vector{…}, ::Vector{…}; sorted::Bool, discretizedomain::Bool, y::SubArray{…}, sortingmode::Symbol)
    This method does not support all of the given keyword arguments (and may not support any).

    Stacktrace:
      [1] kwerr(::@NamedTuple{…}, ::Function, ::Vector{…}, ::Vector{…}, ::Vector{…})
        @ Base ./error.jl:175
      [2] alphabet(X::PropositionalLogiset{…}, sorted::Bool; force_i_variables::Bool, test_operators::Nothing, discretizedomain::Bool, unique::Bool, kwargs::@Kwargs{…})
        @ SoleData ~/Documents/Aclai/SoleData.jl/src/scalar/propositional-logiset.jl:259
      [3] initialize_antecedents(sm::BeamSearch, X::PropositionalLogiset{…}, y::SubArray{…}; discretizedomain::Bool, default_alphabet::Nothing)

this leads me to think that you are also working on SoleData package, correct?
but I didn't found any branch with your mods and I can't test the code.
Please provide it, thanks!

[NickT42]

I'm playing with tests and with every test I ended up with this error:

ERROR: TaskFailedException

    nested task error: MethodError: no method matching _multivariate_scalar_alphabet(::Vector{…}, ::Vector{…}, ::Vector{…}; sorted::Bool, discretizedomain::Bool, y::SubArray{…}, sortingmode::Symbol)
    This method does not support all of the given keyword arguments (and may not support any).

    Stacktrace:
      [1] kwerr(::@NamedTuple{…}, ::Function, ::Vector{…}, ::Vector{…}, ::Vector{…})
        @ Base ./error.jl:175
      [2] alphabet(X::PropositionalLogiset{…}, sorted::Bool; force_i_variables::Bool, test_operators::Nothing, discretizedomain::Bool, unique::Bool, kwargs::@Kwargs{…})
        @ SoleData ~/Documents/Aclai/SoleData.jl/src/scalar/propositional-logiset.jl:259
      [3] initialize_antecedents(sm::BeamSearch, X::PropositionalLogiset{…}, y::SubArray{…}; discretizedomain::Bool, default_alphabet::Nothing)

this leads me to think that you are also working on SoleData package, correct? but I didn't found any branch with your mods and I can't test the code. Please provide it, thanks!

@PasoStudio73 The reason for the errors you're encountering is that I'm running the code on the branch fix/alphabet in SoleData.jl. The function "alphabet" in "SoleData.jl/src/scalar/propositional-logiset.jl" has an issue in main:

function alphabet(
    X::PropositionalLogiset,
    sorted=true;
    force_i_variables::Bool=false,
    test_operators::Union{Nothing,AbstractVector{<:TestOperator},Base.Callable}=nothing,
    discretizedomain::Bool=false,
    unique::Bool=false,
    kwargs...,
)::MultivariateScalarAlphabet
[...]
    domains = [
        begin
            domain = Tables.getcolumn(gettable(X), i_variable(feat))
            if unique && !discretizedomain
                domain = unique(domain)
            end
            domain
        end for feat in feats
    ]
[...]

Here the function call unique(domain) fails because the symbol unique references the local parameter.
I brought this up to @edo-007 a while ago and he fixed it on the branch fix/alphabet, which I've been using up to now to run my code, but it seems like it wasn't merged with the main branch of SoleData.
Try running the code using the branch fix/alphabet and it should run fine.
One last thing: I made another slight modification to the other packages to ensure the correct flow of optional keyword arguments. In SoleModels (main branch) in the file "src/utils/models/other.jl" I added the possibility of passing some kwargs... to the apply function of a DecisionList.
In other words, the lines 132 - 138 of the file in my local version are now

function apply(
    m::DecisionList,
    i::AbstractInterpretation;
    check_args::Tuple = (),
    check_kwargs::NamedTuple = (;),
    kwargs...
)

In "src/utils/models/other.jl" in lines 147 - 153 I did the same. So they are now

function apply(
    m::DecisionList{O},
    d::AbstractInterpretationSet;
    check_args::Tuple = (),
    check_kwargs::NamedTuple = (;),
    kwargs...
) where {O}

Whereas before they didn't allow the passing of kwargs.
I suppose either this will also have to be merged into the main branch of SoleModels at some point, or I will have to modify the test code to make that the parameters passed to apply() are handled directly without passing through optional keyword arguments.

[PasoStudio73]

I'm playing with tests and with every test I ended up with this error:

ERROR: TaskFailedException

    nested task error: MethodError: no method matching _multivariate_scalar_alphabet(::Vector{…}, ::Vector{…}, ::Vector{…}; sorted::Bool, discretizedomain::Bool, y::SubArray{…}, sortingmode::Symbol)
    This method does not support all of the given keyword arguments (and may not support any).

    Stacktrace:
      [1] kwerr(::@NamedTuple{…}, ::Function, ::Vector{…}, ::Vector{…}, ::Vector{…})
        @ Base ./error.jl:175
      [2] alphabet(X::PropositionalLogiset{…}, sorted::Bool; force_i_variables::Bool, test_operators::Nothing, discretizedomain::Bool, unique::Bool, kwargs::@Kwargs{…})
        @ SoleData ~/Documents/Aclai/SoleData.jl/src/scalar/propositional-logiset.jl:259
      [3] initialize_antecedents(sm::BeamSearch, X::PropositionalLogiset{…}, y::SubArray{…}; discretizedomain::Bool, default_alphabet::Nothing)

this leads me to think that you are also working on SoleData package, correct? but I didn't found any branch with your mods and I can't test the code. Please provide it, thanks!

@PasoStudio73 The reason for the errors you're encountering is that I'm running the code on the branch fix/alphabet in SoleData.jl. The function "alphabet" in "SoleData.jl/src/scalar/propositional-logiset.jl" has an issue in main:

function alphabet(
    X::PropositionalLogiset,
    sorted=true;
    force_i_variables::Bool=false,
    test_operators::Union{Nothing,AbstractVector{<:TestOperator},Base.Callable}=nothing,
    discretizedomain::Bool=false,
    unique::Bool=false,
    kwargs...,
)::MultivariateScalarAlphabet
[...]
    domains = [
        begin
            domain = Tables.getcolumn(gettable(X), i_variable(feat))
            if unique && !discretizedomain
                domain = unique(domain)
            end
            domain
        end for feat in feats
    ]
[...]

Here the function call unique(domain) fails because the symbol unique references the local parameter. I brought this up to @edo-007 a while ago and he fixed it on the branch fix/alphabet, which I've been using up to now to run my code, but it seems like it wasn't merged with the main branch of SoleData. Try running the code using the branch fix/alphabet and it should run fine. One last thing: I made another slight modification to the other packages to ensure the correct flow of optional keyword arguments. In SoleModels (main branch) in the file "src/utils/models/other.jl" I added the possibility of passing some kwargs... to the apply function of a DecisionList. In other words, the lines 132 - 138 of the file in my local version are now

function apply(
    m::DecisionList,
    i::AbstractInterpretation;
    check_args::Tuple = (),
    check_kwargs::NamedTuple = (;),
    kwargs...
)

In "src/utils/models/other.jl" in lines 147 - 153 I did the same. So they are now

function apply(
    m::DecisionList{O},
    d::AbstractInterpretationSet;
    check_args::Tuple = (),
    check_kwargs::NamedTuple = (;),
    kwargs...
) where {O}

Whereas before they didn't allow the passing of kwargs. I suppose either this will also have to be merged into the main branch of SoleModels at some point, or I will have to modify the test code to make that the parameters passed to apply() are handled directly without passing through optional keyword arguments.

Thanks for the super quick reply @NickT42, and for all the details you provided me, very very appreciated! This makes sense and I agree with you: those fixes must be merged in main in SoleData, we'll take care do fix that!

[m0rr13]

This pr is huge; better talking about this together.

@NickT42 nice work!