Topic/inverse evaluate proposer #8
Conversation
…lacement of placeholder variables with correct variables
src/trs/rewrite.rs
Outdated
| } | ||
| /// Selects two Rules from the TRS at random and atempts to inverse evaluate one rule on the other, if it | ||
| /// succeeds it takes that new rule and inserts it imediately after the background. | ||
| pub fn inverse_evaluate<R: Rng>(&self, rng: &mut R) -> Result<TRS, SampleError> { |
There was a problem hiding this comment.
Because this is pub, let's make sure to have an example here.
| pub fn inverse_evaluate<R: Rng>(&self, rng: &mut R) -> Result<TRS, SampleError> { | ||
|
|
||
| /// Extracts a sngle rule from the TRS selecting only 1 rhs | ||
| fn sample_rule<R: Rng>(&self, rng: &mut R) -> Result<Rule, SampleError> { |
There was a problem hiding this comment.
Funny - I forgot that I'd put this in the last review and just wrote a similar function, TRS::choose_clause, a couple days ago. I suppose we have a strong use case for it! I'm not sure why this one is quite so long, though. Here's my implementation:
fn choose_clause<R: Rng>(&self, rng: &mut R) -> Result<Rule, SampleError> {
let rules = {
let num_rules = self.len();
let background = &self.lex.0.read().expect("poisoned lexicon").background;
let num_background = background.len();
&self.utrs.rules[0..(num_rules - num_background)]
};
let mut clauses = rules.iter().flat_map(Rule::clauses).collect_vec();
let idx = (0..clauses.len())
.choose(rng)
.ok_or(SampleError::OptionsExhausted)?;
Ok(clauses.swap_remove(idx))
}Am I missing something?
There was a problem hiding this comment.
Oh, I was reading some of the lines you'd deleted as part of the current function. They're pretty similar in length.
| } | ||
| trs.utrs.remove_idx(target_idx).expect("removing old rule"); | ||
| trs.utrs |
There was a problem hiding this comment.
I think you can use ? here instead of expect. That way, you return the error rather than panic. Something like this:
trs.utrs.remove_clauses(&ref_rule)?;| } | ||
| let mut target_idx: usize = rng.gen_range(num_background, num_rules); |
There was a problem hiding this comment.
Instead of sampling a rule by index, would it work just to sample another clause? Also, we write so much boilerplate to figure out how many rules we've learned. Perhaps that could also be extracted into a helper function, say, pub fn learned_rules<'a>(&'a self) -> &'a [Rule] or something similar.
Code to be reviewed, not necessarily merged yet, I have still to finish the documentation and tests.