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

fix: Minify for partial DFAs #184

Merged
merged 5 commits into from
Nov 19, 2023

Conversation

eliotwrobson
Copy link
Collaborator

Resolves #182. Modifies the algorithm to deal with partial DFAs correctly. Somewhat inefficient, but the best we can do until Valmari's algorithm gets implemented (which is a separate open issue).

cc @martinec. If you have any more potentially interesting test cases, would be good to add to make sure the algorithm is working correctly (this one is unfortunately a bit tricky to get right).

@eliotwrobson eliotwrobson self-assigned this Nov 7, 2023
@coveralls
Copy link

coveralls commented Nov 7, 2023

Coverage Status

coverage: 99.64% (+0.001%) from 99.639%
when pulling ecd87c4 on eliotwrobson:partial_fix_2
into ef0e02c on caleb531:develop.

@eliotwrobson eliotwrobson marked this pull request as ready for review November 8, 2023 19:22
Copy link
Contributor

@EduardoGoulart1 EduardoGoulart1 left a comment

Choose a reason for hiding this comment

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

I am currently also struggling with this bug. This LGTM as a temporary fix.

automata/fa/dfa.py Outdated Show resolved Hide resolved
tests/test_dfa.py Outdated Show resolved Hide resolved
@eliotwrobson
Copy link
Collaborator Author

@caleb531 since this is a bug people are actively running into and the volume of changes (outside of the test cases) is small, I'm going to try and merge this sometime today.

@caleb531
Copy link
Owner

caleb531 commented Nov 19, 2023

@eliotwrobson Sure, I have no problem with that—the PR looks fine to me.

And if you want to go ahead and merge to main as well, please feel free. Just please be sure to run through all steps in the Release Checklist (including the creation of the GitHub Release with recent features like the PDA show_diagram method, etc.). I just want to make sure that we have everything captured.

@eliotwrobson
Copy link
Collaborator Author

@caleb531 going to go ahead and merge since you approved.

I don't quite have time to run through the whole release checklist right now, since we added a couple of new features in addition to this bug fix. I'll try and find time in the next couple of days to push out a minor version. I think the main thing is that people who run into this bug have a fixed version available, and just having that on the develop branch should at least help some.

@eliotwrobson eliotwrobson merged commit fa2a6ec into caleb531:develop Nov 19, 2023
7 checks passed
@caleb531
Copy link
Owner

@eliotwrobson Oh okay, sure. If you think having this fix on develop is sufficient, then I'd probably not rather rush a release if we truly don't need to.

@caleb531 caleb531 mentioned this pull request Dec 31, 2023
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.

Minimization of DFA from finite language makes the language infinite
4 participants