Divide and conquer #134
Divide and conquer #134
Conversation
…eHole to RuleNode and remove Union type.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #134 +/- ##
==========================================
+ Coverage 71.26% 75.96% +4.70%
==========================================
Files 20 23 +3
Lines 689 774 +85
==========================================
+ Hits 491 588 +97
+ Misses 198 186 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Project.toml
Outdated
| @@ -5,6 +5,8 @@ version = "0.4.1" | |||
|
|
|||
| [deps] | |||
| DataStructures = "864edb3b-99cc-5e75-8d2d-829cb0a9cfe8" | |||
| DecisionTree = "7806a523-6efd-50cb-b5f6-3fa6f1930dbb" | |||
There was a problem hiding this comment.
I'm wondering whether it'd be nice to follow #124 and make an extension so we don't add DecisionTree as a hard dependency of HerbSearch. The reasoning for doing that with Clingo_jll in #124 is that the package is only used for one specific functionality that not everyone is going to use (grammar refactoring). I'm thinking the divide and conquer strategy is another such case.
| struct ConditionalIfElseError <: Exception | ||
| msg::String | ||
| end | ||
|
|
||
| function Base.showerror(io::IO, e::ConditionalIfElseError) | ||
| print(io, e.msg) | ||
| end |
There was a problem hiding this comment.
Is it necessary to introduce a custom error type if we don't do anything special when printing it? I could see this being useful if you add some context to the error that shows the rule in the grammar that it was looking for (:($sym_bool ? $sym_start : $sym_start)).
| # Turn dic into vector since we cannot guarantee order when iterating over dict. | ||
| ioexamples_solutions = | ||
| [ | ||
| (example, [idx for idx in vec]) for (prob, vec) in problems_to_solutions for |
There was a problem hiding this comment.
| (example, [idx for idx in vec]) for (prob, vec) in problems_to_solutions for | |
| (example, vec) for (prob, vec) in problems_to_solutions for |
| problems_to_solutions::Dict{Problem{Vector{T}}, Vector{Int}}, | ||
| solutions::Vector{RuleNode}, |
There was a problem hiding this comment.
| problems_to_solutions::Dict{Problem{Vector{T}}, Vector{Int}}, | |
| solutions::Vector{RuleNode}, | |
| problems_to_solutions::AbstractDict{Problem{<:AbstractVector{T}}, <:AbstractVector{Int}}, | |
| solutions::Vector{RuleNode}, |
try to keep types abstract so the users don't have to convert to specific Vector/Dict types in case they're using something other than the defaults.
| sym_start::Symbol, | ||
| sym_constraint::Symbol, | ||
| symboltable::SymbolTable, | ||
| )::RuleNode where T <: IOExample |
There was a problem hiding this comment.
Since this type parameter is just used once, it can be removed and the <: IOExample bit can be moved up to the type parameter of problems_to_solutions.
| # We get the first grammar rule that has the specified `sym_constraint` and add constraint to grammar. | ||
| clearconstraints!(grammar) |
There was a problem hiding this comment.
First, copy the grammar here. Otherwise, the divide and conquer process will modify the grammar, and I guess that would be surprising to a user.
| """ | ||
| function get_predicates(grammar::AbstractGrammar, | ||
| sym_bool::Symbol, | ||
| sym_constraint::Symbol, |
There was a problem hiding this comment.
I think if you switch the signature to this, you can remove the second definition of Sorry, didn't look closely enough.get_predicates. The bodies of the methods are the same—right?
Either way, can the logic that follows addconstraint! be rewritten into a single method? This way we don't have to change something twice if we need to make changes to it later on.
| domain = DomainRuleNode(grammar, rules) | ||
| addconstraint!(grammar, ContainsSubtree(domain)) | ||
|
|
||
| iterator = BFSIterator(grammar, sym_bool) | ||
| predicates = Vector{RuleNode}() | ||
|
|
||
| for (i, candidate_program) ∈ enumerate(iterator) | ||
| candidate_program = freeze_state(candidate_program) | ||
| push!(predicates, candidate_program) | ||
| if i >= n_predicates | ||
| break | ||
| end | ||
| end | ||
| return predicates |
There was a problem hiding this comment.
You can replace these lines with a call to the other signature of get_predicates below.
| predicate. | ||
| """ | ||
| function get_features( | ||
| ioexamples_solutions::Vector{Tuple{T, Vector{Int}}}, |
There was a problem hiding this comment.
Do we need the solutions too here? Might be cleaner to pass AbstractVector{<:IOExample} directly.
| @test decide(problem2, expr, symboltable) == false | ||
| end | ||
|
|
||
| @testset verbose = true "conquer" begin |
There was a problem hiding this comment.
Maybe I'm missing it, but you never call the conquer function directly here, right? It looks like the tests dissect the internal logic of the function, but if the function definition is changed, these tests might not catch it. Can you add a test to check it directly?
Description
This PR includes a basic implementation of divide and conquer functions.
Main additions:
divide_and_conquer: Main function implementing the divide-and-conquer search procedure.divide: Breaks down a problem specification into individual subproblems, each consisting of a single input-output example.decide: Decides if a partial program should be kept. Programs are kept if they satisfy at least one subproblem.conquer: Combines solutions to subproblems into a global solution program using a decision tree. Helper functions for decision tree:get_predicates: Generates predicates for feature tests in the decision tree (BFS iterator with provided start symbol).get_features: Constructs feature matrix by evaluating everyIOExampleinputs on all predicates.get_labels: Extracts labels for each subproblem. First partial program that satisfied the subproblem is used as label.construct_final_program: Recursively constructs the final program from the decision tree.