Conversation
…lacement of placeholder variables with correct variables
joshrule
left a comment
joshrule
left a comment
There was a problem hiding this comment.
I know you're not entirely finished with this PR, but here are a few comments on what you've done so far. This is going to be a really cool search move, so I'm glad you're working on it. Looking forward to seeing the completed product!
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.
joshrule
left a comment
joshrule
left a comment
There was a problem hiding this comment.
Cool - you're making progress! I've added a few new comments below. What's the biggest barrier between where you are now and being finished with this feature?
| 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.
| return Err(SampleError::OptionsExhausted); | ||
| } | ||
| 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)?;| if num_background >= num_rules - 1 { | ||
| return Err(SampleError::OptionsExhausted); | ||
| } | ||
| 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.
2 participants
Code to be reviewed, not necessarily merged yet, I have still to finish the documentation and tests.