Added Function of Generating Animation of DFA and NFA Reading Strings

[dsr20030703]

I have added a animate_reding_input function in DFA and NFA to automatically generate the animation of the DFA or NFA reading the input using manim 0.19.0.

I have also added a Jupyter Notebook for the funciton in the example_notebooks/ folder to show the generated animation.

As there're some problems in the type lint of manim and pygraphviz, which includes:

1. Argument color to manim.FadeToColor expected str, but it also accepts type ManimColor;
2. The docstring of pygraphviz.Node and pygraphviz.Edge says each node or edge has attributes that can be directly accessed through the attr dictionary, but they aren't recognized by mypy.

It comes with some errors in typechecking run with mypy. But there aren't any functional bugs (as far as I have checked).

Another problem I have encountered is that I don't know how to add the tests. As manim is an animation engine, they have developed a unique test framework based on pytest, which is different from this project. So I haven't added any test. (But at least the examples in the Jupyter Notebook all works, except the last of which)

[eliotwrobson]

@dsr20030703 thanks for this contribution! I'm a bit busy this week, but I'll try and get a review in this weekend 👍

[eliotwrobson]

Several comments. My main reservation is having certain class definitions behind if statements, but the code itself seems solid. Should consult with @caleb531 to see what the best way to organize these changes is. Overall looks good though, thanks again for this contribution!!

[eliotwrobson]

@dsr20030703 the overall structure of the code looks good now, but it looks like tests are failing. If you can get the tests passing, then I can do a full review 👍🏽

[dsr20030703]

It seems that checks failed when installing dependencies because some additional dependencies are required to install on Linux. Should I add a new step after "Setup graphviz" in .github/workflows/build.yml like this?

- name: Build Additional Dependencies for manim
  run: |
    sudo apt update
    sudo apt install -y build-essential python3-dev libcairo2-dev libpango1.0-dev

[eliotwrobson]

@dsr20030703 go for it 👍

[caleb531]

@eliotwrobson @dsr20030703 Apologies for the very late response from me.

I'm gonna be completely honest, but my initial impression is that animation feels out-of-scope for this package. Looking over the PR, it feels like it's going to add a lot of weight that I think would be better suited for a fork. However, I am open to discussion on this point.

Also, out of curiosity, can the output directory be customized? It feels rather restrictive to have the generated files be restrictively written to a media/ directory.

[dsr20030703]

@caleb531

I'm gonna be completely honest, but my initial impression is that animation feels out-of-scope for this package. Looking over the PR, it feels like it's going to add a lot of weight that I think would be better suited for a fork. However, I am open to discussion on this point.

Will adding a new optional-dependencies like animation be acceptable? The result of the current visualization function, FA.show_diagram, when provided with a long input_str, frankly speaking, may suck. For example, the following code

dfa = DFA(
    states={"q0", "q1", "q2", "q3"},
    input_symbols={"0", "1"},
    transitions={
        "q0": {"0": "q2", "1": "q1"},
        "q1": {},
        "q2": {"0": "q3", "1": "q1"},
        "q3": {"0": "q2", "1": "q1"},
    },
    initial_state="q0",
    final_states={"q3"},
    allow_partial=True,
)
dfa.show_diagram("1111111111111")

will produce such a graph

Also, out of curiosity, can the output directory be customized? It feels rather restrictive to have the generated files be restrictively written to a media/ directory.

Yes, there're a lot of configurations in manim. Users can customize the output directory with manim render --custom_folders.

[caleb531]

Will adding a new optional-dependencies like animation be acceptable? The result of the current visualization function, FA.show_diagram, when provided with a long input_str, frankly speaking, may suck. For example, the following code
...
Yes, there're a lot of configurations in manim. Users can customize the output directory with manim render --custom_folders.

Is it truly an optional dependency, though? Because I also saw your comment about needing to globally install Manim😕

It seems that checks failed when installing dependencies because some additional dependencies are required to install on Linux. Should I add a new step after "Setup graphviz" in .github/workflows/build.yml like this?

- name: Build Additional Dependencies for manim
  run: |
    sudo apt update
    sudo apt install -y build-essential python3-dev libcairo2-dev libpango1.0-dev

I am mostly concerned about how people who don't need the animation functionality will potentially be required to install these sub-dependencies (or global dependencies, whatever you want to call them). But if we are able to work around this through the use of optional dependencies / addons (like we have the [visual] addon already), then I'd still consider myself open to this functionality.

cc @eliotwrobson

[eliotwrobson]

I am mostly concerned about how people who don't need the animation functionality will potentially be required to install these sub-dependencies (or global dependencies, whatever you want to call them). But if we are able to work around this through the use of optional dependencies / addons (like we have the [visual] addon already), then I'd still consider myself open to this functionality.

I could be wrong, but I think the way this PR is set up, the global dependencies are only needed if you're using the animation (i.e. someone could just install automata without the visual extra and everything would work fine), but we need to install everything in CI to run tests. I definitely agree though that there should be a way to make this additional functionality optional.

[dsr20030703]

It seems that build.yml, lint.yml and tests.yml shall be changed simultaneously when adding new dependencies. As the first 4 steps of them are same, could those steps be extracted and shared by 3 jobs so that just 1 place need to be changed?

Also, it seems that this project is going to adopt uv? It would help a lot in setting up since Manim also recommends using uv. Shall I wait until #254 gets reviewed and merged?

[caleb531]

It seems that build.yml, lint.yml and tests.yml shall be changed simultaneously when adding new dependencies. As the first 4 steps of them are same, could those steps be extracted and shared by 3 jobs so that just 1 place need to be changed?

@dsr20030703 Are you familiar with how to consolidate shared steps among GHA workflows like this? I'm not as familiar, but if you are, I would love a PR for that sort of thing. The duplication between the files bothers me too.

Also, it seems that this project is going to adopt uv? It would help a lot in setting up since Manim also recommends using uv. Shall I wait until #254 gets reviewed and merged?

I would say yes, probably. @eliotwrobson, do you have any thoughts on this?

[eliotwrobson]

@caleb531 I agree, I think #254 is pretty close as well, so we should be able to close that out soon. I need a little bit of input from you there since I'm not 1000% how you want the deployment set up, and there's ways to consolidate the number of actions run if we would like.

[eliotwrobson]

@dsr20030703 sorry for the delay on our end. Just merged the UV PR to the develop branch, so you should be good to rebase + update the code here.

[dsr20030703]

@eliotwrobson I'm a newbie on git. How should I rebase and update code? 😥 I asked ChatGPT. The answer is like this:

git remote add caleb531 https://github.com/caleb531/automata.git
git fetch caleb531
git rebase caleb531/develop
# ... fix conflicts
git push origin animation --force

The problem is that the prior commits has been uploaded to "origin/animation" branch on GitHub. Before the last line of command, the local "animation" branch and the "origin/animation" branch on GitHub will separate. So the last line of command will discard the prior commits on "origin/animation" and replace them with new local commits. Is that OK? A senior of mine in school once told me: "there's only 1 occasion that you need to use --force in git -- You're gonna to be fired." 😰

[eliotwrobson]

The commands you posted are generally the correct way to do a rebase. You can take a look at this article as well: https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase

The problem is that the prior commits has been uploaded to "origin/animation" branch on GitHub. Before the last line of command, the local "animation" branch and the "origin/animation" branch on GitHub will separate. So the last line of command will discard the prior commits on "origin/animation" and replace them with new local commits. Is that OK?

If you're concerned about local / remote branches being out of sync, you should git pull before doing a rebase. If you do this first, I think it should be fine. But yes, a rebase does get rid of the old commits and replace them with the new ones, which is how the conflicts get resolved.

A senior of mine in school once told me: "there's only 1 occasion that you need to use --force in git -- You're gonna to be fired." 😰

This is not correct, force pushes are a standard part of a rebase workflow, and many places strongly prefer this over merge commits to make it easier to do code review. This can be true if your organization doesn't have branch protection enabled, but we have this as part of this project. In short, by doing this you won't wipe out anybody else's work.

[coveralls]

coverage: 99.544% (-0.07%) from 99.613%
when pulling fd15fd9 on dsr20030703:animation
into c477062 on caleb531:develop.

[dsr20030703]

It seems that manim wasn't installed when creating virtual environment in "Run mypy"? 😕 I just ran command uv add --optional visual manim to add the requirements for "manim" to "visual" option (also add the steps in those 3 yaml files) in commit 34b3b74, then rebased to that commit. @eliotwrobson How did you made the pyproject.toml file? Are there any other configurations have to be made to let manim be installed during "Run mypy"?

[eliotwrobson]

@dsr20030703 the error you're seeing is because the manim package is installed, but the library doesn't export the type annotations it has (I think, it looks like there isn't a py.typed file in their github: https://github.com/ManimCommunity/manim). Maybe you can import from the typing module explicitly? https://docs.manim.community/en/stable/contributing/docs/types.html

It might be worth adding an issue on their repo to get some clarity on what's going on. Worst case, you can add manim to the ignore list, which isn't a huge deal since the tests with this library are running + passing: https://github.com/caleb531/automata/blob/develop/pyproject.toml#L86-L94

[dsr20030703]

I’ve opened an issue there. While I’ve also found that there has been an issue criticizing the code of manim (though feeling that it may not have a good title 😓) years ago. As seems that it may take time to complete the issue, shall we add manim to the ignore list first?

Another problem I encountered in type checking is what discussed here. The return type of partial is partial[M] which conforms to Callable[…, M]. But in that case, one can’t get the argument list of the initializing function of MObject when coding, which would decrease the help of type annotation. Are there any better solutions? 🤔

[eliotwrobson]

As seems that it may take time to complete the issue, shall we add manim to the ignore list first?

I think that's the way to go. We can revisit this list later if manim ever figures things out.

Another problem I encountered in type checking is what discussed here. The return type of partial is partial[M] which conforms to Callable[…, M]. But in that case, one can’t get the argument list of the initializing function of MObject when coding, which would decrease the help of type annotation. Are there any better solutions? 🤔

I see. I think it would be fine to use a type ignore statement or similar because the use of partial erases the type annotation. Otherwise, I think it's ok to change the return type to something like Callable[..., M] and return a function defined within the method you're seeing the type error in that does the same thing as the partial call with argument forwarding. Either way, I don't think that this function is going to be used too much outside of first party code, so I don't think it's a huge deal to have a potentially inaccurate annotation for this.

[dsr20030703]

If you can get the tests passing, then I can do a full review 👍🏽

So we can begin the full review? By clicking “Ready for review” button?

[eliotwrobson]

A few small questions but overall the code looks really good! Two questions:

• Would it be possible to add some short discussion to the documentation? This can just point to the notebook since that's a more comprehensive example, but some mention of that would be good.
• Is there a way to double check that everything works without having manim installed? It would be good to make sure that we don't accidentally break anything because of the new optional dependency.

Want to check with @caleb531 to see what he thinks about the changes here but overall I think this has a significant value add for people using the library in jupyter notebooks.

[dsr20030703]

• Would it be possible to add some short discussion to the documentation? This can just point to the notebook since that's a more comprehensive example, but some mention of that would be good.

There have been links to the example_notebooks folder in docs/index.md and README.md. Or perhaps we can rewrite the docs/examples/fa-examples.md according to the current 2 Jupyter Notebooks? After all, Jupyter Notebooks can't run directly on a webpage, but videos can. And we can use HTML <video> tag to embed videos in Markdown.

• Is there a way to double check that everything works without having manim installed? It would be good to make sure that we don't accidentally break anything because of the new optional dependency.

Currently, the public functions (i.e. whose not starting with a _ and the name of the class it belongs, if there is, also doesn't start with a _) that needs manim, are only the animate_reading_input function in DFA and NFA class. And I've added the checks for manim in these 2 functions.

[eliotwrobson]

There have been links to the example_notebooks folder in docs/index.md and README.md.

Ahh I forgot that this linked directly to the folder. I think it's fine for now then.

Currently, the public functions (i.e. whose not starting with a _ and the name of the class it belongs, if there is, also doesn't start with a _) that needs manim, are only the animate_reading_input function in DFA and NFA class. And I've added the checks for manim in these 2 functions.

That sounds good I think, we don't have to write an automated test and it seems like the impact is pretty minimal.