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

Added show_diagram method for Push Down Automata #177

Merged
merged 20 commits into from
Oct 23, 2023

Conversation

Vipul-Cariappa
Copy link
Contributor

@Vipul-Cariappa Vipul-Cariappa commented Oct 13, 2023

Implemented the show_diagram method for Push Down Automata.
I have just adapted the FA.show_diagram (Finite Automata) code for PDA.show_diagram.

I have not yet implemented show_diagram with input_str. I will require some suggestions here. Should I also show the operations performed on the stack, or only the state transitions are sufficient?

I have not tested the code, except for DPDA and NPDA in the docs. I have attached the images below.

DPDA:
image

NPDA:
image

Reminder for myself and the maintainer: I should also update the docs to reflect the new feature, before merging this PR.

Hope this is helpful. Please let me know if any changes are required.


Edited:

I have implemented the construction of the diagram with an input_str.

DPDA from docs with input_str = "aabb":
image

NPDA from docs with input_str = "abba":
image

I have updated the docs. I should now write down some test cases.

@coveralls
Copy link

coveralls commented Oct 13, 2023

Coverage Status

coverage: 99.639% (-0.2%) from 99.847% when pulling 510e471 on Vipul-Cariappa:pda-visualizer into 35b68fd on caleb531:develop.

docs/pda/class-pda.md Outdated Show resolved Hide resolved
automata/pda/npda.py Outdated Show resolved Hide resolved
@caleb531
Copy link
Owner

@Vipul-Cariappa To answer your question, I would personally prefer that the stack operation show for each transition because in principle, you want the diagram to be a visual representation of the machine. And changes to the stack can drastically affect the machine's behavior for a PDA.

@eliotwrobson Do you have any thoughts on any of this?

@Vipul-Cariappa
Copy link
Contributor Author

@caleb531 I have updated the code to also display the stack operations. Results:

DPDA from docs with input_str = "aabb":
image

NPDA from docs with input_str = "abba":
image

automata/pda/pda.py Outdated Show resolved Hide resolved
automata/pda/pda.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@eliotwrobson eliotwrobson left a comment

Choose a reason for hiding this comment

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

Left some early comments. Overall looks good, but needs some test cases and I think some things can be simplified significantly. Thanks for the contribution @Vipul-Cariappa !

automata/pda/configuration.py Outdated Show resolved Hide resolved
automata/pda/pda.py Outdated Show resolved Hide resolved
automata/pda/pda.py Outdated Show resolved Hide resolved
automata/pda/pda.py Outdated Show resolved Hide resolved
automata/pda/pda.py Show resolved Hide resolved
@eliotwrobson
Copy link
Collaborator

@Vipul-Cariappa To answer your question, I would personally prefer that the stack operation show for each transition because in principle, you want the diagram to be a visual representation of the machine. And changes to the stack can drastically affect the machine's behavior for a PDA.

I'm in agreement @caleb531 , it's nice to be able to generate the states of the stack along with the visual representation of the machine. @Vipul-Cariappa, it would be awesome to have a flag enabling displaying only the machine or only the stack (both by default).

@eliotwrobson eliotwrobson changed the base branch from main to develop October 15, 2023 00:40
@Vipul-Cariappa
Copy link
Contributor Author

it would be awesome to have a flag enabling displaying only the machine or only the stack (both by default).

@eliotwrobson, I have implemented the feature you asked for.


I have refactored the code to remove duplications, to the best of my abilities, according to what I felt makes sense.

Please have a look at it, and let me know if any other changes are required.

Two things left to do:

  • Write test cases
  • Update the md doc file for with_machine and with_stack parameters

automata/pda/dpda.py Outdated Show resolved Hide resolved
automata/pda/npda.py Outdated Show resolved Hide resolved
@Vipul-Cariappa
Copy link
Contributor Author

I have added test cases. Again, I adapted test cases from dfa and nfa. Please review it and let me know if any more changes are required.

Copy link
Collaborator

@eliotwrobson eliotwrobson left a comment

Choose a reason for hiding this comment

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

New changes are looking good, in particular the tests seem fairly comprehensive! Left comments in a few places, I think some logic can be simplified significantly and there were a few things I had questions about.

automata/base/utils.py Outdated Show resolved Hide resolved
automata/pda/pda.py Outdated Show resolved Hide resolved
automata/fa/fa.py Outdated Show resolved Hide resolved
automata/pda/npda.py Outdated Show resolved Hide resolved
automata/pda/pda.py Show resolved Hide resolved
automata/pda/pda.py Show resolved Hide resolved
automata/pda/pda.py Outdated Show resolved Hide resolved
automata/pda/pda.py Show resolved Hide resolved
@Vipul-Cariappa
Copy link
Contributor Author

I have made some of the changes @eliotwrobson suggested. Also, left some clarifications and questions.
Please have a look at it.

@Vipul-Cariappa
Copy link
Contributor Author

I guess all change requests are implemented. Please have a look. Let me know if I missed any.

Copy link
Collaborator

@eliotwrobson eliotwrobson left a comment

Choose a reason for hiding this comment

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

Don't want to approve quite yet (as I'd like a little more time to review everything, and requested a few changes), but this is getting very close to being ready to merge!

automata/base/utils.py Outdated Show resolved Hide resolved
automata/pda/pda.py Outdated Show resolved Hide resolved
automata/pda/pda.py Outdated Show resolved Hide resolved
automata/pda/npda.py Outdated Show resolved Hide resolved
automata/pda/npda.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@eliotwrobson eliotwrobson left a comment

Choose a reason for hiding this comment

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

I want to double check whatever is going on with the closing bracket in the unresolved discussion, but the code looks good!

EDIT: Resolved the above. @caleb531 I would say go ahead and merge if you think everything looks good. Would prefer to have another set of eyes before merging just because of the volume of changes.

@caleb531
Copy link
Owner

@eliotwrobson I won't have time to review it today, but I can probably look tomorrow or Monday. In the meantime, could you please add some tests for the NotImplemented / NotImplementedError cases? That will bring the coverage back up, and there should be some existing tests you can use as reference.

Copy link
Owner

@caleb531 caleb531 left a comment

Choose a reason for hiding this comment

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

@eliotwrobson @Vipul-Cariappa This PR looks very good! I only have one requested change (well, technically two, I suppose) related to the new DiagramException class. But if one of you could please make those couple changes quick, I'll be ready to give this the final sign-off!

Thanks also to both of you for all your hard work on this PR so far.

automata/base/exceptions.py Outdated Show resolved Hide resolved
Copy link
Owner

@caleb531 caleb531 left a comment

Choose a reason for hiding this comment

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

@eliotwrobson Thanks for that! Everything looks good to me now. Approving and merging...

@caleb531 caleb531 merged commit b469f41 into caleb531:develop Oct 23, 2023
6 of 7 checks passed
@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.

4 participants