Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes an error related to depth-limit in grammars without terminal rules #28

Merged
merged 3 commits into from Oct 21, 2020
Merged

Fixes an error related to depth-limit in grammars without terminal rules #28

merged 3 commits into from Oct 21, 2020

Conversation

ancorso
Copy link
Contributor

@ancorso ancorso commented Oct 21, 2020

When we have a grammar where not every type has a terminal rule, setting a depth limit when sampling expressions can cause an error. For example, the grammar

grammar = @grammar begin
        A = B
        B = C
        C = D
        D = E
        E = 1
    end

will cause an error when we sample with a depth limit of 1 i.e. rulenode = rand(RuleNode, grammar, :A, 1).

I changed the logic so that if the depth limit is reached and a terminal rule is available then choose that rule. If no terminal rules are available then it chooses from the available rules. The example above is added as a test.

@ancorso ancorso requested a review from rcnlee October 21, 2020 13:51
@coveralls
Copy link

coveralls commented Oct 21, 2020

Coverage Status

Coverage decreased (-1.3%) to 86.667% when pulling 3002dab on ancorso:depth_lim_iss into 8573b55 on sisl:master.

@mykelk
Copy link
Member

mykelk commented Oct 21, 2020

Looks good to me!

@rcnlee
Copy link
Collaborator

rcnlee commented Oct 21, 2020

Thanks Anthony! So instead of throwing an error, it'll return the incomplete expression and the user should handle it on their side (e.g., in the reward function)? How will the user know that the expression is incomplete and need special handling?

The other question I had was regarding performance, since rand is a core inner loop function. The filtering is now happening at every step (rand is recursive) even when majority of the time maxdepth > 1 and it is not needed. Perhaps we can restructure the if-statement to avoid that.

@ancorso
Copy link
Contributor Author

ancorso commented Oct 21, 2020

Hi Ritchie,

It does complete the expression, the expression may just have a depth larger than max_depth. The goal is to always output a valid expression.

As for performance, I can rework the if statement so that the filtering only happens when we reach the max depth. It'll just be an extra line or two.

@rcnlee
Copy link
Collaborator

rcnlee commented Oct 21, 2020

Sounds good, I agree that is the best way to handle it. I'll merge when the checks complete. Thanks Anthony!

@rcnlee rcnlee merged commit 0dcf7fa into sisl:master Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants