DecisionList integration in SoleXplorer#15
Open
PasoStudio73 wants to merge 83 commits into
Open
Conversation
IREP* implementation in src/algorithms/sequentialcovering.jl was fixed after beamsearch was changed to use the Antecedent structure. Also the optimizations made to IREP* made in the branch labels have been merged to irepnewtypes before rewriting IREP_Star to use the Antecedent structure most efficiently
The method unaryconditions_noneq has been replaced with the more efficient method alphabet2conditions, and the computation has been modified to only occurr once for the entire execution
the irepstar function in sequentialcovering.jl has been fixed after it was broken during code refactoring
Some basic code has been added to do some sanity check on the results of irepstar
the target_class parameter in beamsearch's findbestantecedent has been applied correctly. Some comments explaining the code and some docs have been added, a couple of features that were either disabled due to refactoring or bugs have been fixed, the first practical irepstar test has proven successful. The laplace_accuracy function in loss_functions.jl has been updated to also handle different types of labeling
…sts branch The development on this branch was started from a non-updated commit of RandomDecisionLists. With this commit, I reintegrated the work I had done after the commit off of which this branch was started. Namely, there are a couple bug fixes with symmetric loss functions, adaptation to the new loss function system in the MLJ interfaces, some new parameter checks. Another difference is that the weight parameter can now be passed as a symbol again to the different sequential covering methods, and lastly the tests on basic sequential covering have been updated
|
This PR started from a slightly older version of RandomDecisionLists, but don't worry, I’ve already integrated the latest changes in [12bbc24]. Since those changes didn't really overlap with your edits, everything merged cleanly. I did remove the ensemble code in my latest work (moving it to the SoleModels PR), but keeping them here for now is fine. We can migrate them later once the featurenames changes are ready. Just a heads-up: the Also, as for your question about Overall, the code looks good to me! Nice work. |
giopaglia
reviewed
Apr 19, 2026
Comment on lines
+256
to
+295
| function DecisionListClassifier(; | ||
| searchmethod::SearchMethod=BeamSearch(), | ||
| tdl_threshold::Int=64, | ||
| split_ratio::Real=0.7, | ||
| loss_function::LossFunctions.AsymmetricLoss=LossFunctions.LaplaceAccuracy(), | ||
| max_infogain_ratio::Union{Nothing,Real}=nothing, | ||
| default_alphabet::Union{Nothing,AbstractAlphabet}=nothing, | ||
| discretizedomain::Bool=false, | ||
| significance_alpha::Union{Real,Nothing}=0.0, | ||
| min_rule_coverage::Int=1, | ||
| max_rule_length::Union{Nothing,Int}=nothing, | ||
| max_rulebase_length::Union{Nothing,Int}=nothing, | ||
| # BeamSearch | ||
| conjuncts_generation_method::AbstractGenerator=AtomGenerator(), | ||
| beam_width::Int=3, | ||
| # utils | ||
| rng::AbstractRNG=TaskLocalRNG(), | ||
| suppress_parity_warning::Bool=false, | ||
| ) | ||
| model = DecisionListClassifier( | ||
| searchmethod, | ||
| tdl_threshold, | ||
| split_ratio, | ||
| loss_function, | ||
| max_infogain_ratio, | ||
| default_alphabet, | ||
| discretizedomain, | ||
| significance_alpha, | ||
| min_rule_coverage, | ||
| max_rule_length, | ||
| max_rulebase_length, | ||
| conjuncts_generation_method, | ||
| beam_width, | ||
| rng, | ||
| suppress_parity_warning, | ||
| ) | ||
| message = MMI.clean!(model) | ||
| isempty(message) || @warn message | ||
| return model | ||
| end |
Member
There was a problem hiding this comment.
We could try to avoid these code repetitions around struct and constructor definition? MLJ seems to provide a dedicated macro for this https://juliaai.github.io/MLJModelInterface.jl/dev/quick_start_guide/#Model-type-and-constructor
giopaglia
reviewed
Apr 19, 2026
Comment on lines
+316
to
+330
| searchmethod=m.searchmethod, | ||
| tdl_threshold=m.tdl_threshold, | ||
| split_ratio=m.split_ratio, | ||
| loss_function=m.loss_function, | ||
| max_infogain_ratio=m.max_infogain_ratio, | ||
| default_alphabet=m.default_alphabet, | ||
| discretizedomain=m.discretizedomain, | ||
| significance_alpha=m.significance_alpha, | ||
| min_rule_coverage=m.min_rule_coverage, | ||
| max_rule_length=m.max_rule_length, | ||
| max_rulebase_length=m.max_rulebase_length, | ||
| conjuncts_generation_method=m.conjuncts_generation_method, | ||
| beam_width=m.beam_width, | ||
| rng=m.rng, | ||
| suppress_parity_warning=m.suppress_parity_warning |
Member
There was a problem hiding this comment.
slightly better maybe
Suggested change
| searchmethod=m.searchmethod, | |
| tdl_threshold=m.tdl_threshold, | |
| split_ratio=m.split_ratio, | |
| loss_function=m.loss_function, | |
| max_infogain_ratio=m.max_infogain_ratio, | |
| default_alphabet=m.default_alphabet, | |
| discretizedomain=m.discretizedomain, | |
| significance_alpha=m.significance_alpha, | |
| min_rule_coverage=m.min_rule_coverage, | |
| max_rule_length=m.max_rule_length, | |
| max_rulebase_length=m.max_rulebase_length, | |
| conjuncts_generation_method=m.conjuncts_generation_method, | |
| beam_width=m.beam_width, | |
| rng=m.rng, | |
| suppress_parity_warning=m.suppress_parity_warning | |
| m.searchmethod, | |
| m.tdl_threshold, | |
| m.split_ratio, | |
| m.loss_function, | |
| m.max_infogain_ratio, | |
| m.default_alphabet, | |
| m.discretizedomain, | |
| m.significance_alpha, | |
| m.min_rule_coverage, | |
| m.max_rule_length, | |
| m.max_rulebase_length, | |
| m.conjuncts_generation_method, | |
| m.beam_width, | |
| m.rng, | |
| m.suppress_parity_warning |
giopaglia
reviewed
Apr 19, 2026
Comment on lines
+441
to
+462
| num_models=m.num_models, | ||
| use_bootstrapping=m.use_bootstrapping, | ||
| samples_ratio_per_model=m.samples_ratio_per_model, | ||
| n_subfeatures_per_model=m.n_subfeatures_per_model, | ||
| aggregation_function=m.aggregation_function, | ||
| model_wrapper=irepstar, | ||
| rng=m.rng, | ||
| # irepstar kwargs | ||
| searchmethod=m.searchmethod, | ||
| tdl_threshold=m.tdl_threshold, | ||
| split_ratio=m.split_ratio, | ||
| loss_function=m.loss_function, | ||
| max_infogain_ratio=m.max_infogain_ratio, | ||
| default_alphabet=m.default_alphabet, | ||
| discretizedomain=m.discretizedomain, | ||
| significance_alpha=m.significance_alpha, | ||
| min_rule_coverage=m.min_rule_coverage, | ||
| max_rule_length=m.max_rule_length, | ||
| max_rulebase_length=m.max_rulebase_length, | ||
| conjuncts_generation_method=m.conjuncts_generation_method, | ||
| beam_width=m.beam_width, | ||
| suppress_parity_warning=m.suppress_parity_warning |
Member
There was a problem hiding this comment.
same here
Suggested change
| num_models=m.num_models, | |
| use_bootstrapping=m.use_bootstrapping, | |
| samples_ratio_per_model=m.samples_ratio_per_model, | |
| n_subfeatures_per_model=m.n_subfeatures_per_model, | |
| aggregation_function=m.aggregation_function, | |
| model_wrapper=irepstar, | |
| rng=m.rng, | |
| # irepstar kwargs | |
| searchmethod=m.searchmethod, | |
| tdl_threshold=m.tdl_threshold, | |
| split_ratio=m.split_ratio, | |
| loss_function=m.loss_function, | |
| max_infogain_ratio=m.max_infogain_ratio, | |
| default_alphabet=m.default_alphabet, | |
| discretizedomain=m.discretizedomain, | |
| significance_alpha=m.significance_alpha, | |
| min_rule_coverage=m.min_rule_coverage, | |
| max_rule_length=m.max_rule_length, | |
| max_rulebase_length=m.max_rulebase_length, | |
| conjuncts_generation_method=m.conjuncts_generation_method, | |
| beam_width=m.beam_width, | |
| suppress_parity_warning=m.suppress_parity_warning | |
| m.num_models, | |
| m.use_bootstrapping, | |
| m.samples_ratio_per_model, | |
| m.n_subfeatures_per_model, | |
| m.aggregation_function, | |
| irepstar, | |
| m.rng, | |
| # irepstar kwargs | |
| m.searchmethod, | |
| m.tdl_threshold, | |
| m.split_ratio, | |
| m.loss_function, | |
| m.max_infogain_ratio, | |
| m.default_alphabet, | |
| m.discretizedomain, | |
| m.significance_alpha, | |
| m.min_rule_coverage, | |
| m.max_rule_length, | |
| m.max_rulebase_length, | |
| m.conjuncts_generation_method, | |
| m.beam_width, | |
| m.suppress_parity_warning |
giopaglia
reviewed
Apr 19, 2026
| loss_function = pop!(args_dict, :loss_function) | ||
| min_rule_coverage = pop!(args_dict, :min_rule_coverage) | ||
|
|
||
| irepstar(X, y, target_class; rng = rng, loss_function = loss_function, min_rule_coverage = min_rule_coverage, args_dict...) |
Member
There was a problem hiding this comment.
we can use this cleaner syntax in a bunch of places I think.
Suggested change
| irepstar(X, y, target_class; rng = rng, loss_function = loss_function, min_rule_coverage = min_rule_coverage, args_dict...) | |
| irepstar(X, y, target_class; rng, loss_function, min_rule_coverage, args_dict...) |
Then it becomes clearer that this method is just ignoring three kwargs. A possible alternative:
function model_wrapper(X, y, rng; rng, loss_function, min_rule_coverage, kwargs...)
irepstar(X, y, target_class; , kwargs...)
end
Member
|
Hey Gio, thanks for all the comments! Just to clarify, even though this PR targets main, it's not the one where all those changes were included. As Trombini pointed out in his comment, this branch actually predates that work. I'd recommend taking a look at #14 for more context. That said, we'll definitely take all of your feedback into account and address it in the dedicated PR. |
The current RIPPER implementation was optimizing the ruleset on the uncovered data, whereas in reality it should do so on the entire dataset. This has been fixed
Member
Author
|
@NickT42 , I see your last commit, checked also in SoleXplorer and there's no compatibility issues, good catch! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I'm working on integrating RandomDecisionList in SoleXplorer, working on MLJ interface for this model.
I need you help in irepstar function: to let
apply!works, I had to changedefault_consequentin order to havesupporting_labelstoo. I ended giving tosupporting_labelsa static valuey, but I don't know if this is correct, or alters the behaviour of irepstar.Please help!!!