-
Notifications
You must be signed in to change notification settings - Fork 70
Visual Automata #124
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
Comments
@lewiuberg So I've been thinking about this ever since you posted the question here, and to be honest, I'm still rather undecided. But there have been a few questions running through my mind:
And then the other consideration for me has been whether visualization should really be part of the scope of this library. I know I accepted the @eliotwrobson @abhinavsinha-adrino What do you guys think? |
So glancing at the visual automata code, it looks like the main extensions to the core library are expanded versions of the @caleb531 maybe if there were a way to set up optional dependencies that would be better? Some way of making the visualization code optional for those that need it. |
Good morning @caleb531 and @eliotwrobson!
@eliotwrobson Yes, it is mostly expanding on I also think I can swap the dependency |
@lewiuberg @eliotwrobson Considering that the DFA class in this project has grown substantially over the past year (mostly from new DFA operations), I am hesitant to add much more to it. Having reviewed the Visual-Automata code myself, it seems like it would add a substantial weight to both the DFA and NFA classes. I personally feel that would make it more difficult to navigate the code in those classes. In other words, I am against integrating the Visual Automata code with the DFA/NFA classes themselves. However, I'd like to propose an alternative solution: Create a dedicated I also considered adding dedicated subclasses like Thoughts? 😁 |
@caleb531 I think that's a good proposal. The visualization code can use shared logic between DFAs and NFAs (and possibly GNFAs) with some minor modifications. As you stated, it will also compartmentalize rendering code, allowing the use of optional dependencies and reducing the size of the respective files. |
@eliotwrobson Okay, great! I forgot to mention that the other implication of my proposal would be that the existing @lewiuberg When you have a a chance, and can review the proposed architecture, you are welcome to submit a PR whenever you find the time. |
Hi @caleb531 (@eliotwrobson) I am about to start. Have I understood correctly, that you want the visualizer class to be placed at from automata.fa.dfa import DFA
from automata.fa.visualizer import Visualizer
visual_dfa = Visualizer.from_dfa(dfa)
print(visual_dfa.table) I have not looked much at this in a couple of years, so I need some time to get back into it. If I have misunderstood, please let me know; and an example of how you want it to operate would be helpful :) |
@lewiuberg I think that interface is what @caleb531 was getting at. I think we can decide on more fine-grained parts of the class later. |
@lewiuberg @eliotwrobson After thinking about it, I agree that |
@lewiuberg Now that #129 and #149 have been merged, I think we've integrated all of the features from the visual automata library here! I don't think visual automata can be marked as deprecated yet (as we have not released the next version of automata), but we should be able to do that once the next release happens. Feel free to take a look at the develop branch and see if there are any missing features or things you think should be added. If you think everything looks good, we can probably close this issue. |
Just had a look now! Great work! I really like the grouping 👍
I saw this now that you guys didn't want to hardcode/bind it to a specific style. Its up to you, but I think its a pity to drop it 😞 Great work! |
@lewiuberg if you find it useful and are able, we'd be happy to add it as a short example in the same way as others. I just didn't do it earlier because of lack of time and issues with the code being really messy (and that it gives a very similar output to read input stepwise). Otherwise, glad to hear it! |
I could add a PR for that tomorrow maybe? If I understood you correctly? I would have done more earlier, but work and kids kind of took over 😅 |
@lewiuberg that would be awesome! You should be able to model your changes after what was done in #149. |
@eliotwrobson Life happened 😅 Submitted a PR now. Hope I understood you correctly 👍🏻 |
@lewiuberg automata v8, including all of the features in the previous discussion, has been released! You can now safely deprecate |
@lewiuberg have you had a chance to finalize this deprecation? No worries if you have too much going on, but it would be great to close this out soon (as fall classes will get going before too long). |
Hi @eliotwrobson ! Sorry! I've been on vacation with the kids :) Let me just understand, you want me to make an update to the visual-automata library, so it gives a deprecation warning from now on? |
@lewiuberg no worries! I hope you had a nice vacation 😃 A deprecation warning isn't necessary, I think all that's needed is to add a deprecation message (linking back to this repo) to the README.md and marking the visual-automata repo as read-only, so people cannot create new issues / pull requests. |
@eliotwrobson Hi! Sorry again! Too many things going on. I have done this now. |
@lewiuberg no worries and thanks! I'll go ahead and close. |
Hi!
Would you consider implementing the Visual Automata wrapper directly into automata-lib and deprecating Visual Automata?
The text was updated successfully, but these errors were encountered: