Skip to content

fix: Fix bug in minify edge case #218

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

Merged
merged 4 commits into from
May 16, 2024
Merged

Conversation

eliotwrobson
Copy link
Collaborator

Fixes small bug in the minify function for partial DFAs and the as_partial check for a DFA creation function.

I think this raises the question, @caleb531 should the allow_partial be a computed property in the constructor if it is manually set by the user? i.e., if client code sets this property to true, should it be set to False if the transition dictionary is not partial.

@coveralls
Copy link

Coverage Status

coverage: 99.613% (+0.001%) from 99.612%
when pulling 0251de6 on eliotwrobson:fixes
into 530016a on caleb531:develop.

@caleb531
Copy link
Owner

@eliotwrobson If the user explicitly sets allow_partial, then the machine should honor that choice. I'd prefer not to traverse the entire transition map as a computed property—that feels expensive. I can see a use case where the user passes in a variable transition map which may or may not be partial (perhaps it depends on user input).

@eliotwrobson
Copy link
Collaborator Author

@caleb531 that line of reasoning makes sense. The reason I bring this up is because the to_complete method relies on the allow_partial only being set to true if the DFA is indeed partial (otherwise it adds an extra state, which is what was causing the bug here in the first place):

https://github.com/caleb531/automata/blob/develop/automata/fa/dfa.py#L341

I think the way to go then is to do the check for being partial at this function call, instead of relying on this boolean property. What do you think? This discussion is a bit orthogonal to the point of this PR, so I'll go ahead and merge. Thanks for the fast approval!

@eliotwrobson eliotwrobson merged commit 3b029cf into caleb531:develop May 16, 2024
@caleb531
Copy link
Owner

@eliotwrobson Yes, I think that is the approach to take. For me, the allow_partial parameter ultimately represents user choice, not the guaranteed state of the machine (hence the name—allow_partial, not is_partial). Therefore, it should not be relied upon as an assumption of the actual state of the machine (using the term "state" loosely here).

I suppose in theory, we could drop allow_partial entirely and just compute at instantiation time whether the machine is partial or not. But again, that sounds rather expensive to me, and also makes current non-partial validation checks pointless (since effectively this would allow every machine to be partial and get away with it).

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.

3 participants