Bottom up iterator #107
Bottom up iterator #107
Conversation
… into bottom-up-iterator
… into bottom-up-iterator
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #107 +/- ##
==========================================
+ Coverage 67.35% 74.78% +7.43%
==========================================
Files 21 26 +5
Lines 729 944 +215
==========================================
+ Hits 491 706 +215
Misses 238 238 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
src/bottom_up_iterator.jl
Outdated
| current_programs::Queue{RuleNode} | ||
| mutable struct BottomUpState | ||
| bank::Any | ||
| data::Any |
There was a problem hiding this comment.
non-urgent thing to think about: should we leave bank and data as Any, or should we introduce some subtyping? I assume the bank will always be either a Dict so that programs are ordered over their type, or some kind of priority as in Brute. If we figure out more precise type, the code might be faster.
There was a problem hiding this comment.
I have decided to introduce two new abstract types: BUDepthBank and BUDepthData that the concrete iterators will need to extend. Please let me know what you think about this.
src/bottom_up_iterator.jl
Outdated
| iter::DepthIterator | ||
| )::Dict{Symbol, Vector{RuleNode}} | ||
| grammar::ContextSensitiveGrammar = get_grammar(iter.solver) | ||
| bank::Dict{Symbol, Vector{RuleNode}} = Dict{Symbol, Vector{RuleNode}}() |
There was a problem hiding this comment.
does it make sense to make the bank type Dict{Symbol, Dict{Int,Vector{RuleNode}}}? so that you can also index it over depth for easier fetching?
src/bottom_up_iterator.jl
Outdated
| grammar::ContextSensitiveGrammar = get_grammar(iter.solver) | ||
| rules::Queue{Int} = Queue{Int}() | ||
|
|
||
| for (rule_index, is_terminal) ∈ enumerate(grammar.isterminal) |
There was a problem hiding this comment.
there might already be a function to get all non-terminals in HerbGrammar. If not, maybe we should add it?
… being enabled by default.
|
@TudorMagirescu: |
Just to trigger CI for now...