Closed lewiuberg closed 1 year ago
@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 DFA.show_diagram
contribution several years ago, but in retrospect, I've had mixed feelings about its being-part the library overall. I suppose it may be practical because DFAs are common, but I wonder if we open a can of worms and inflate the library substantially by implementation visualization methods for other automaton subtypes (especially if they're only used by a small subset of this library's user base).
@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 show_diagram
functions for DFA and NFA. Since the base version of show_diagram
is already a part of this library, I don't think it's really a huge stretch to include the functionality in visual automata (assuming we can keep the external dependencies somewhat trimmed down). A lot of the other functionality in visual automata should be able to be integrated relatively easily with the code here after the functionality we added in v7. @lewiuberg is my line of thinking correct here?
@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!
automata-lib
; without bringing anything new or improved to the table.nfa
and dfa
. I was going to do more, but I could not find the time with work, my wife, and my kids.@eliotwrobson Yes, it is mostly expanding on show_diagram
, but there are some methods working in the background to visualize the path taken from the user's input. And there is a method that makes a table with the correct symbols and a table that lets the user check if their input is accepted or rejected.
I also think I can swap the dependency jupyterlab
with IPython
.
@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 Visualizer
class with multiple constructor methods like from_dfa
and from_nfa
. That way, the visualization stays separate from the data structure manipulation logic that 99% of the class is comprised of. It should also allow us to make the visualization dependencies optional, since you should only encounter errors if you try to import the Visualizer class without one of those dependencies installed.
I also considered adding dedicated subclasses like automata.fa.visual_dfa.VisualDFA
. However, I don't want to imply that a VisualDFA is a theoretical concept at the same level of a DFA or NFAβhence why I propose a sort of higher-level utility class to consume existing Automaton instances that do fit within the theory.
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 DFA.show_diagram
would be removed from the DFA class, since that logic will be moved to and become wholly part of the Visualizer class.
@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 automata.fa.visualizer
?
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 automata.fa.visualizer
makes the most sense (i.e. scoped to the fa
subpackage), because visualizing something like a Turing machine or NPDA could be done quite differently than DFA/NFA, so there's less potential for logic to be shared anyway.
@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 see that the nfa.table functionality has not been implemented. Any reason why? I found it very useful when I went to school π
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 visual-automata
. Once you add the deprecation message to your readme and mark the repo read-only, I will close this issue.
@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?