Added jupyter notebook integration and new visualization #129
Conversation
|
@khoda81 This is really cool! I haven't looked at the code super closely, but would it be possible to use this Jupyter integration as part of an optional dependency, as in the discussion here: #124 I think that it would be really nice to have the changes here accompany a move towards the architecture described in the issue above. |
|
@eliotwrobson Looking at Visual Automata seems like I reinvented the wheel. I wish it were mentioned in the project readme. I agree that We can add Visual Automata as an optional dependency, then surround the import with a try-except block; just like it is done in many libraries like flask. The rest is just replacing my visualizations in the FA class with Visual Automata ones and printing a warning if the import failed and falling back to the default repr of the class. This way the only code added to this library would be only a few lines of code in the FA class, with a few utility methods ( This would also remove the need for the If my rough explanations made sense and this is what you expected, I will implement these changes. |
|
@khoda81 I think the way you refactored this is definitely more towards the direction that I think we want to go in with respect to the visualizations described in that issue. Instead of adding Visual Automata as a dependency, this would be a great opportunity to start to integrate that code with your refactors in here (since the purpose of that original issue was to move Visual Automata into this library). As you stated, with your refactors, the size of the code will decrease significantly. @caleb531 do you have any input? |
|
@eliotwrobson @khoda81 I am hesitant about adding Visual Automata as a dependency because it would duplicate all of the non-visualization Automata code. So I am more in favor of integrating the code into this library. While I do like my I suppose if we nix the Visualizer class, I will again raise my concern about the DFA/NFA classes becoming too bloated, but I also want to recognize that practicality beats purity. Also cc'ing @lewiuberg on this thread so that he's in the loop about this proposed new direction, especially since he was already supposedly starting on the Visualizer implementation for #124. @lewiuberg, if you have any thoughts on this, please feel free to chime in. |
|
@eliotwrobson @caleb531 Alright then, not adding Visual Automata as a dependency makes this implementation a whole lot easier, it's just a matter of stealing the visualizations from Visual Automata and replacing mine with them. from automata.fa.dfa import DFA
dfa = DFA(
states={'q0', 'q1', 'q2'},
input_symbols={'0', '1'},
transitions={
'q0': {'0': 'q0', '1': 'q1'},
'q1': {'0': 'q0', '1': 'q2'},
'q2': {'0': 'q2', '1': 'q1'},
},
initial_state='q0',
final_states={'q1'}
)
dfa
dfa.minify(retain_names=True)
It would act just like it did before everywhere else and prints the result of |
|
Hi guys! First, sorry for being a bit on/off. I have had a crazy period at work. I have been working on this from time to time, but since automata isn't really something that affects me in my work the direction we decide on isn't that important to me. If you guys want to "steal" the code or add it as a dependency it is fine by me. I just feel it is very beneficial for learning purposes and I don't want all the time I invested into it to go to waste. Although I think it would be the most effective to implement it to automata-lib in terms of maintenance. One cool thing I want to share is that I just spoke to a professor at Colombia University, and they are now starting to use Visual-Automata in their classes 😃 |
That's outstanding! That's actually part of why I really like this PR, the integration with something like Jupyter makes it much easier to create self-contained course content using this library. |
|
@khoda81 This is a great start so far! Thank you so much for what you are contributing. I do have a few points of constructive feedback:
|
|
@khoda81 Just a heads up—for more consistent code style across the entire codebase, I plan to integrate the black auto-formatter and apply it to all Python files in the library. Once I do this on the |
caleb531
force-pushed
the
jupyter-integration
branch
2 times, most recently
from
cf81d69
to
2be4d6c
Compare
|
@khoda81 Okay, I've added black to the I've done my best to resolve conflicts and preserve the integrity of your changes, but please double-check just to be sure. |
|
@caleb531 black is a great addition! It would be cool to add it as a pre-commit hook. I might do that after this PR. It would help greatly with contributions. I usually use |
@khoda81 So I have limited knowledge of pre-commit hooks, but from what I understand, they can't be committed to the repository directly and require some manual setup to activate. So it seems like the pre-commit hook is only good for those contributors who take the time to set it up. I guess the same could be said of editor plugins. In your experience, is there a good way to streamline the management/adoption of pre-commit hooks for a project? cc @eliotwrobson (in case you have any thoughts on this) |
|
@caleb531 To add them to this project we need to only add the |
|
@khoda81 This project does have a CONTRIBUTING file, but it's under the https://github.com/caleb531/automata/blob/develop/.github/CONTRIBUTING.md Anyway, you've persuaded me that a pre-commit hook would be a valuable addition to this project. I would greatly appreciate a separate PR for this if you have the time to contribute one. |
|
@caleb531 my bad, I somehow missed the file. I will add pre-commit hooks in a separate PR. |
|
@eliotwrobson @caleb531
The first solution is more on brand with the rest of this PR but would add another method for the purpose of visualization which I have been trying to avoid. The upside would be that if implemented properly it can replace |
|
@khoda81 For option 1, do you mean that the method would return a data structure holding information about each step that is taken, and then the visualization code could just read this directly? Does this differ greatly from just yielding all results from the generator given by |
|
@khoda81 I think any major re-architecture of the input-reading APIs will require some iteration and proof-of-concepts, for sure. On that note, I like the idea of starting with the second option since it seems like it would have less of an API impact, and would be a good starting point to explore larger changes. Out of curiosity, what kinds of detail is missing from the current input-reading API that a new API would be more useful in providing? |
|
@caleb531 @eliotwrobson My proposal for the new API is here #138. BTW, I have a hard time figuring out what the purpose of |
|
Step visualizations are implemented and seem to be working properly. I am torn between different ways to label transitions in a way not to make the diagram cluttered but concise. |
|
@khoda81 I think it's always going to be a bit tricky visualizing D/NFAs reading input much longer than their number of states (inherently things are going to get cluttered, it would be better to have an animation rather than a static image but that's way out of scope I think). I like the way you have the color grading become more green as more of the input is being read. Maybe try italicizing the Could you try an example with a somewhat larger DFA constructed using the I haven't looked at the code, but it seems like a few of the backend changes might overlap a bit with #139, and this branch is already bit out of date with |
Per the documentation, @functools.cache "returns the same as lru_cache(maxsize=None)". See: <https://docs.python.org/3/library/functools.html#functools.cache>
|
@caleb531 @khoda81 I've pushed changes 10706a6 that fix the correctness issue I was seeing (at least when testing with the small example NFA), along with tests verifying this for that particular instance. There have been a few uncovered lines that have been exposed, but the test cases are checking for most of the important issues related to correctness. The preceding issue was the last major obstacle to this PR. All that remains is to add a few test cases that ensure correctness and cover the last remaining lines. I should be able to attack some of these tests, but would greatly appreciate some help on this, as there are a few pesky uncovered lines that require some weird setup code to hit. I also want to be sure about the optimality of the new accepting path finding algorithm. EDIT: After making another pass, all that's left is to test that everything works even with the optional imports. |
|
@eliotwrobson @khoda81 At this point, I'm tempted to just bypass the slight drop in coverage (to address in a separate PR) for the sake of merging this PR. It will unblock #147 and any other future PRs that touch these files (which would often be the case). I've also been realizing that achieving a perfect 100% code coverage can be impractical or even unattainable, sometimes. @eliotwrobson I think my only requirement for wrapping up this PR at this point would be to work through the remaining comment threads to reach resolution for all of them. |
|
@caleb531 sounds like a plan. I closed all of the conversations related to changes that I already made. There are only 4 remaining ones open, and they're all fairly tangential or something that we can resolve with testing later (minor stuff related to how the project is configured). I'll make another pass. |
|
@caleb531 I've resolved all of the open comment threads on this PR. The only dangling items are:
Other than those, we have very high coverage and tests for most everything that's been added. Once you give the code here a quick lookover / are comfortable, I think we're good to go ahead and merge! |
There was a problem hiding this comment.
@eliotwrobson @khoda81 This works for me! I've just approved everything, so I'll go ahead and merge now.
Changes in this PR
Examples of visual changes
More Example images
images.zip
The ones not included in the old folder resulted in error when generating.
Note: Tests for the pydot graph need to be updated. I am waiting for these new visualizations to be approved and finalized first.