Added Function of Generating Animation of DFA and NFA Reading Strings #252

dsr20030703 · 2025-03-18T16:44:34Z

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:

Argument color to manim.FadeToColor expected str, but it also accepts type ManimColor;
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)

Add requirements for manim.

Added the docstring for animate_reading_input function. Removed the FAAnimation class. Handle the case when the input is empty in animate_reading_input function.

eliotwrobson · 2025-03-18T18:44:32Z

@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 · 2025-03-24T17:12:52Z

.gitignore

@@ -24,3 +24,6 @@ nosetests.xml

 # MyPy
 .mypy_cache
+
+# manim generated files
+media/


Should have a newline at the end of the file.

I've added a new line at the end of this file in the newest commit (6149742). And Git at local also has recorded the change. But I don't know why it's missed when uploading to the GitHub. 😕❓

I think you have to configure VSCode to add newlines when saving any file, otherwise you might see weird behavior like this.

eliotwrobson · 2025-03-24T17:18:41Z

automata/base/animation.py

+
+    DEFAULT_COLOR = manim.WHITE
+    HIGHLIGHT_COLOR = manim.RED
+    manim.Circle.set_default(color=DEFAULT_COLOR)


Does this change a global property of manim? Is there a way to do this local to what is animated from this class?

Yes, it changes the default color of manim.Circle from manim.RED to manim.WHITE globally. A way to do this local to what is animated from this class is by specifying color=Animate.DEFAULT_COLOR in every manim.MObject created, which is what I did at first. After that, I thought it may be too troublesome. As most of manim.MObject except manim.Circle uses manim.WHITE as default color, I chose to change the default color of manim.Circle to manim.WHITE globally.

Is the way of specifying color=Animate.DEFAULT_COLOR in every manim.MObject created suggested? If so, I'll revise the code in that way.

Is the way of specifying color=Animate.DEFAULT_COLOR in every manim.MObject created suggested? If so, I'll revise the code in that way.

I think this should be possible through inheritance or a helper function? You could have an object creation function that creates and returns the objects with these defaults set.

Is a helper function like this OK? (partial is from functools)

Suggested change

manim.Circle.set_default(color=DEFAULT_COLOR)

M = TypeVar("M", bound=manim.Mobject)

@classmethod

def default_init(cls, mobject_class: type[M]):

return partial(mobject_class, color=cls.DEFAULT_COLOR)

It can handle all subclasses of manim.Mobject like manim.Circle, manim.Text, etc. In that case, we would use Animate.default_init(/*a manim.Mobject class*/) to replace the default init function of that class. For example, manim.Text(node.name, font_size=float(node.attr["fontsize"])) will be replaced by Animate.default_init(manim.Text)(node.name, font_size=float(node.attr["fontsize"])), and manim.Circle() will be repalced by Animate.default_init(manim.Circle)() (seems that the latter () are easy to forget though).

I think that seems reasonable! My main motivation for this comment is to avoid side effects that can cause other animation code to behave unexpectedly, and I think this is a solid way of doing that.

EDIT: Be sure to annotate the return type here as well (I think it is also type[M]).

eliotwrobson · 2025-03-24T17:18:59Z

automata/base/animation.py

+    shape: manim.Dot | manim.Circle | manim.VGroup
+    label: Optional[manim.Text]
+
+    def __init__(self, node: pgv.Node):


Suggested change

def __init__(self, node: pgv.Node):

def __init__(self, node: pgv.Node) -> None:

eliotwrobson · 2025-03-24T17:19:50Z

automata/base/animation.py

+            self.add(self.shape)
+            self.label = manim.Text(node.name, font_size=float(node.attr["fontsize"]))
+            self.add(self.label)
+        x, y = (float(pt) / 72 for pt in node.attr["pos"].split(","))


72 is a magic number here, could this be set at the class level or at the top of the file?

72 is the ratio between inch and point (actually, big boint). Graphviz measures some attributes of Graph, Node and Edge in points, while some attibutes are measured in inches.

Is adding a constant like _POINTS_IN_INCH = 72 at the top of the file, and replacing all "72" with "_POINTS_IN_INCH" OK?

Yes, I think that's a good strategy 👍🏽

eliotwrobson · 2025-03-24T17:21:53Z

automata/base/animation.py

+    string are separated with each character is generated to a `Text` object, so that
+    you can get each character simply with `[]` operator."""
+
+    def __init__(self, text: str):


Suggested change

def __init__(self, text: str):

def __init__(self, text: str) -> None:

eliotwrobson · 2025-03-24T17:23:54Z

automata/fa/dfa.py

+
+if fa._visual_imports:
+
+    @dataclass(eq=False)


Should we put this in the animation file, or make a separate folder for just animations? I'm not a fan of having an entire class definition behind an if statement like this, but not 100% sure what the best way to do this is. @caleb531 any thoughts on this?

As _DFAAnimation and _NFAAnimation requires DFA and NFA respectively, and the animate_reading_input function in DFA and NFA needs to call the initialize function of _DFAAnimation and _NFAAnimation respectively, it seems that circular dependencies can't be avoided. :( If we put them into other files, like a new automata/fa/animation.py file, then we have to put the import statement inside the animate_reading_input function to prevent circular import, like this:

class DFA: ... def animate_reading_input(self, input_str: str, preview: bool = False) -> None: if not fa._visual_imports: raise ImportError(...) from automata.fa.animation import _DFAAnimation _DFAAnimation(self, input_str).render(preview)

Is that acceptable?

Let me think about this some and get back to you. Circular dependencies like these can usually be resolved by changing the design somehow.

Also, I think the circular dependency may only be because of the DFA / NFA type annotation? In that case, you can put those imports behind an if TYPE_CHECKING guard, see here for discussion.

Also, I think the circular dependency may only be because of the DFA / NFA type annotation? In that case, you can put those imports behind an if TYPE_CHECKING guard, see here for discussion.

It works! Now we don't need to import the module when calling.

eliotwrobson · 2025-03-24T17:24:49Z

example_notebooks/fa_animation.ipynb

Thanks for adding the example notebook!

eliotwrobson · 2025-03-29T18:29:34Z

automata/fa/nfa.py

+        if not fa._visual_imports:
+            raise ImportError(
+                "Missing visualization packages; "
+                "please install pygraphviz, coloraide, and manim."


I think you still need to add manim to the visual imports section in fa.py. I think it would also be good to add a helper function that checks for the visual imports and raises an exception if they are not present (i.e. just put this if block into a helper function in fa.py).

In spite of that manim isn't used in fa.py? Plus, I don't think that checking for the visual imports every time when calling them is a good idea, as the environment won't change when the program is running. Perhaps setting a variable recording the result of checking for the visual imports using importlib in the automata.base.configs would be better? In that case, we don't need to "try-import-except ImportError" to check visual imports every time, just coding

if config._visual_imports: import ...

is OK. Plus, we could let the _visual_imports to be a Optional[ImportError] object. When visual imports exists, its value is None; while when visual imports doesn't exist, its value is ImportError("Missing visualization packages; please install ...") (while in that case, it may be named as miss_visual_imports). So that we don't need to change all the notes of ImportErrors when we want to change the visualization packages.

@dsr20030703 I think that idea makes sense. My suggestion was mainly to try to avoid repeating this message, so using the optional import error would accomplish this 👍🏽

eliotwrobson · 2025-03-31T18:16:32Z

@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 · 2025-04-01T08:37:00Z

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 · 2025-04-01T13:31:07Z

@dsr20030703 go for it 👍

eliotwrobson · 2025-04-01T16:15:34Z

requirements.txt

Missing a newline, and could you add the new dependency in alphabetical order with the rest?

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 · 2025-04-01T17:24:08Z

@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 · 2025-04-01T20:19:27Z

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 · 2025-04-02T00:46:30Z

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.

Made requirements.txt in alphabetical order.

dsr20030703 · 2025-04-04T01:41:53Z

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 · 2025-04-04T23:30:02Z

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 · 2025-04-05T00:13:51Z

@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.

dsr20030703 · 2025-04-05T05:09:36Z

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.

I’m not familiar with that either. 😢 Seems that we can combine these 3 files into 1 file, then extract the 4 common steps into a new job, like setup, then let the other 3 jobs needs the setup job. So the combined yaml file would be like this:

name: CI

…

jobs:
  setup:
    …

    steps:
      # the common 4 steps

  build:
    needs: setup
    …

  lint:
    needs: setup
    …

  tests:
    needs: setup
    …

dsr20030703 added 2 commits March 18, 2025 14:20

Add animation for DFA and NFA reading strings.

d1bfb02

Add requirements for manim.

Made classes for generating animations of automata private.

751745f

Added the docstring for animate_reading_input function. Removed the FAAnimation class. Handle the case when the input is empty in animate_reading_input function.

eliotwrobson reviewed Mar 24, 2025

View reviewed changes

Revised the code according to the review.

6149742

eliotwrobson requested changes Mar 29, 2025

View reviewed changes

Optimized checking visualization imports.

c082e7c

eliotwrobson reviewed Apr 1, 2025

View reviewed changes

requirements.txt Outdated

Copy link

Collaborator

eliotwrobson Apr 1, 2025

Choose a reason for hiding this comment

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

Missing a newline, and could you add the new dependency in alphabetical order with the rest?

caleb531 reviewed Apr 1, 2025

View reviewed changes

Added a step to build dependencies for manim.

fcdb161

Made requirements.txt in alphabetical order.

-    manim.Circle.set_default(color=DEFAULT_COLOR)
+    M = TypeVar("M", bound=manim.Mobject)
+    @classmethod
+    def default_init(cls, mobject_class: type[M]):
+        return partial(mobject_class, color=cls.DEFAULT_COLOR)

	def __init__(self, node: pgv.Node):
	def __init__(self, node: pgv.Node) -> None:

	def __init__(self, text: str):
	def __init__(self, text: str) -> None:

Added Function of Generating Animation of DFA and NFA Reading Strings #252

Are you sure you want to change the base?

Added Function of Generating Animation of DFA and NFA Reading Strings #252

Conversation

dsr20030703 commented Mar 18, 2025

eliotwrobson commented Mar 18, 2025

eliotwrobson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliotwrobson Mar 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliotwrobson commented Mar 31, 2025

dsr20030703 commented Apr 1, 2025

eliotwrobson commented Apr 1, 2025

Choose a reason for hiding this comment

caleb531 left a comment

Choose a reason for hiding this comment

dsr20030703 commented Apr 1, 2025 • edited Loading

caleb531 commented Apr 1, 2025

eliotwrobson commented Apr 2, 2025

dsr20030703 commented Apr 4, 2025

caleb531 commented Apr 4, 2025

eliotwrobson commented Apr 5, 2025

dsr20030703 commented Apr 5, 2025

eliotwrobson Mar 28, 2025 •

edited

Loading

dsr20030703 commented Apr 1, 2025 •

edited

Loading