Graph assumes that first two arguments of edge_type.__init__ are start and end parameters

[mdrost]

Description of bug / unexpected behavior

When trying to use Graph with changed edge_type to LabeledLine (default is Line) I got error TypeError: LabeledLine.__init__() got multiple values for argument 'label'.
After some debugging I found out that Graph._populate_edge_dict method assumes that first two arguments of edge_type.__init__ are start and end parameters:

    def _populate_edge_dict(
        self, edges: list[tuple[Hashable, Hashable]], edge_type: type[Mobject]
    ):
        self.edges = {
            (u, v): edge_type(
                self[u].get_center(),
                self[v].get_center(),
                z_index=-1,
                **self._edge_config[(u, v)],
            )
            for (u, v) in edges
        }

After I changed the code to following one the problem seems to vanish:

    def _populate_edge_dict(
        self, edges: list[tuple[Hashable, Hashable]], edge_type: type[Mobject]
    ):
        self.edges = {
            (u, v): edge_type(
                start=self[u].get_center(),
                end=self[v].get_center(),
                z_index=-1,
                **self._edge_config[(u, v)],
            )
            for (u, v) in edges
        }

Expected behavior

Graph should accept any edge_type based on Line class.

How to reproduce the issue

Code for reproducing the problem

from manim import *
class GraphWithLabeledLine(Scene):
    def construct(self):
        g = Graph(
            vertices = [0, 1],
            edges = [(0, 1)],
            edge_type = LabeledLine,
            edge_config = {
                (0, 1): {'label': 'DERP'},
            },
        )
        self.add(g)
        self.wait()

Logs

Terminal output

PS C:\git\manimations> uv run manim -v DEBUG -pql main.py GraphWithLabeledLine
Manim Community v0.19.0

╭─────────────────────────────── Traceback (most recent call last) ────────────────────────────────╮
│ C:\git\manimations\.venv\Lib\site-packages\manim\cli\render\commands.py:125 in render            │
│                                                                                                  │
│   122 │   │   │   try:                                                                           │
│   123 │   │   │   │   with tempconfig({}):                                                       │
│   124 │   │   │   │   │   scene = SceneClass()                                                   │
│ ❱ 125 │   │   │   │   │   scene.render()                                                         │
│   126 │   │   │   except Exception:                                                              │
│   127 │   │   │   │   error_console.print_exception()                                            │
│   128 │   │   │   │   sys.exit(1)                                                                │
│                                                                                                  │
│ C:\git\manimations\.venv\Lib\site-packages\manim\scene\scene.py:237 in render                    │
│                                                                                                  │
│    234 │   │   """                                                                               │
│    235 │   │   self.setup()                                                                      │
│    236 │   │   try:                                                                              │
│ ❱  237 │   │   │   self.construct()                                                              │
│    238 │   │   except EndSceneEarlyException:                                                    │
│    239 │   │   │   pass                                                                          │
│    240 │   │   except RerunSceneException:                                                       │
│                                                                                                  │
│ C:\git\manimations\main.py:4 in construct                                                        │
│                                                                                                  │
│    1 from manim import *                                                                         │
│    2 class GraphWithLabeledLine(Scene):                                                          │
│    3 │   def construct(self):                                                                    │
│ ❱  4 │   │   g = Graph(                                                                          │
│    5 │   │   │   vertices = [0, 1],                                                              │
│    6 │   │   │   edges = [(0, 1)],                                                               │
│    7 │   │   │   edge_type = LabeledLine,                                                        │
│                                                                                                  │
│ C:\git\manimations\.venv\Lib\site-packages\manim\mobject\graph.py:656 in __init__                │
│                                                                                                  │
│    653 │   │   │   │   self._edge_config[e] = copy(default_edge_config)                          │
│    654 │   │                                                                                     │
│    655 │   │   self.default_edge_config = default_edge_config                                    │
│ ❱  656 │   │   self._populate_edge_dict(edges, edge_type)                                        │
│    657 │   │                                                                                     │
│    658 │   │   self.add(*self.vertices.values())                                                 │
│    659 │   │   self.add(*self.edges.values())                                                    │
│                                                                                                  │
│ C:\git\manimations\.venv\Lib\site-packages\manim\mobject\graph.py:1545 in _populate_edge_dict    │
│                                                                                                  │
│   1542 │   │   self, edges: list[tuple[Hashable, Hashable]], edge_type: type[Mobject]            │
│   1543 │   ):                                                                                    │
│   1544 │   │   self.edges = {                                                                    │
│ ❱ 1545 │   │   │   (u, v): edge_type(                                                            │
│   1546 │   │   │   │   self[u].get_center(),                                                     │
│   1547 │   │   │   │   self[v].get_center(),                                                     │
│   1548 │   │   │   │   z_index=-1,                                                               │
╰──────────────────────────────────────────────────────────────────────────────────────────────────╯
TypeError: LabeledLine.__init__() got multiple values for argument 'label'

[Akshat-Mishra-py]

I would like to work on this! This will be my first contribution in an Open Source.

Proposed fix:
In Graph._populate_edge_dict, I plan to set the edges as follows:

self.edges = {
    (u, v): edge_type(
        start=self[u].get_center(),
        end=self[v].get_center(),
        z_index=-1,
        **self._edge_config[(u, v)],
    )
    for (u, v) in edges
}

This should make the graph support LabeledLine as an edge type, not just Line.

Question:
Should I also implement the same change in DiGraph to keep things consistent and avoid possible conflicts in the future?

Any feedback or suggestions are appreciated, as this is my first open-source contribution!

[mdrost]

I didn't test DiGraph and other types of graphs, axis and other classes accepting Lines, but if there are problems there it would be most welcome to also fix them. It would be also nice to expand unit tests.

[Akshat-Mishra-py]

Certainly, I will add new unit tests for Graph, DiGraph to test both Line and LabledLine any other test case that needs to be added feel free to tell me

[mdrost]

Thank you.
If it doesn't strain your kindness I also see that Graph tests lacks testing of custom vertex_type e.g. LabeledDot. It should be tested with default and with custom vertex_config.

[Akshat-Mishra-py]

Yes, I will add that test and open the PR tomorrow with all completed changes, Thank You for your guidance

[mdrost]

After some thought I have some thoughts (:D) that testing edge cases like LabeledDot with default vertex_config that depends on labels parameters might be unnecessary depending on implementation details and be an example of Hyrum's Law. Second opinion from hardened manim developers would be nice.

[mdrost]

I also discovered that equivalent problem is not only during graph creation but also on adding new edges later in GenericGraph._add_edge:

edge_mobject = edge_type(
    self[u].get_center(), self[v].get_center(), z_index=-1, **edge_config
)

[Akshat-Mishra-py]

Thanks For Clarifying, but I already changed this as it was important to remove any errors that come later on.
And a thought that graph was never meant to be used with LabeledLine and was written like that but edge_type was there for further updates to it and wasn't complete. I do think that this will complete this feature. If you find any other issues please do tell, Thanks Again.