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

BUGFIX Earley: Now yielding a previously repressed ambiguity #1427

Merged
merged 5 commits into from
Jun 22, 2024
Merged

Conversation

erezsh
Copy link
Member

@erezsh erezsh commented Jun 18, 2024

Contains 3 commits:

  1. Test test_multiple_start_solutions, that fails before the fix

  2. Fixes Earley. Previously, slightly different items were considered equal.

  3. A fix to the test_cycle2, which I believe was previously wrong

Issue #1424 was the first lead to finding this bug. But seems that it isn't solved by this change alone.

@chanicpanic Can you please verify my work?

@erezsh
Copy link
Member Author

erezsh commented Jun 18, 2024

P.S. It's easier to see why there should be ambiguity in test_cycle2, if we rename _recurse to recurse. The second recurse is now taken, and was previously ignored. (i.e. it's a new path, not an extra cycle)

Copy link
Contributor

@chanicpanic chanicpanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a couple optional comments.

While this doesn't fully solve the issue from #1424, it does appear to fix all the examples in #536.

lark/parsers/earley.py Outdated Show resolved Hide resolved
lark/parsers/earley.py Outdated Show resolved Hide resolved
@erezsh
Copy link
Member Author

erezsh commented Jun 21, 2024

@chanicpanic Like that?

@chanicpanic
Copy link
Contributor

Yup, looks good

@erezsh
Copy link
Member Author

erezsh commented Jun 22, 2024

@MegaIng Any comments? Or should I just merge it?

Copy link
Member

@MegaIng MegaIng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't say much on the actual implementation changes since I don't know the earley algorithm well enough, but looks good in general.

lark/parsers/earley.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.54%. Comparing base (1be22db) to head (efeb846).
Report is 8 commits behind head on master.

Files Patch % Lines
lark/parsers/earley.py 80.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1427      +/-   ##
==========================================
+ Coverage   89.53%   89.54%   +0.01%     
==========================================
  Files          52       52              
  Lines        7814     7825      +11     
==========================================
+ Hits         6996     7007      +11     
  Misses        818      818              
Flag Coverage Δ
unittests 89.54% <90.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@erezsh erezsh merged commit c1dbe0c into master Jun 22, 2024
22 checks passed
@erezsh
Copy link
Member Author

erezsh commented Jun 22, 2024

Thanks @chanicpanic and @MegaIng !

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.

4 participants