Skip to content

fix(graph): support edge lookup via tuple key in GenericGraph.__getitem__#4753

Open
sridhar-3009 wants to merge 1 commit into
ManimCommunity:mainfrom
sridhar-3009:fix/graph-getitem-edge-support
Open

fix(graph): support edge lookup via tuple key in GenericGraph.__getitem__#4753
sridhar-3009 wants to merge 1 commit into
ManimCommunity:mainfrom
sridhar-3009:fix/graph-getitem-edge-support

Conversation

@sridhar-3009
Copy link
Copy Markdown

Summary

Fixes #3798.

GenericGraph.__getitem__ only looked up vertices. When a user passed a
(u, v) tuple to retrieve an edge mobject, it raised a KeyError against
self.vertices instead of looking up self.edges.

g = Graph([1, 2, 3, 4], [(1, 2), (2, 3)])
g[1]       # works — returns vertex mobject
g[(1, 2)]  # before: KeyError; after: returns edge mobject

Changes

manim/mobject/graph.py:

  • Route 2-tuple keys to self.edges in __getitem__; all other keys
    continue to look up self.vertices

tests/module/mobject/test_graph.py:

  • Add test_graph_getitem covering vertex lookup, edge lookup on
    Graph, and edge lookup on DiGraph

Test plan

  • pytest tests/module/mobject/test_graph.py::test_graph_getitem

GenericGraph.__getitem__ only handled vertex lookup. Passing a 2-tuple
(u, v) to retrieve an edge mobject raised a KeyError against the
vertices dict instead of looking up self.edges.

Route tuple keys of length 2 to self.edges so that g[(u, v)] returns
the edge mobject, consistent with how edges are stored internally.

Adds a test covering vertex lookup, edge lookup (Graph), and edge
lookup (DiGraph).

Fixes ManimCommunity#3798
Copy link
Copy Markdown
Member

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this and adding tests! I left one comment which is necessary to address:

Comment thread manim/mobject/graph.py
Comment on lines 672 to 675
def __getitem__(self: Graph, v: Hashable) -> Mobject:
if isinstance(v, tuple) and len(v) == 2:
return self.edges[v]
return self.vertices[v]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One thing I mentioned in issue #3798 is that vertices can be any hashable object. This includes tuples: vertices can be tuples themselves. This is not too strange, actually. For example, vertices could represent 2D coordinates. There's also this concept of a "line graph L of a graph G" where each vertex of L is an edge of G: if there is an edge connecting vertices 1 and 3 of G, then L has a vertex (1, 3), and so on. networkx actually implements this.

It is possible that the graph contains two vertices, say, (1, 2) and (1, 3), and an edge connecting both: ((1, 2), (1, 3)). Thus, verifying that v is a tuple containing two elements is not enough to determine whether it represents an edge or a vertex.

One way I would solve this is by directly asking whether v is contained in the vertices or the edges before attempting to retrieve the Mobject:

if v in self.vertices:
    return self.vertices[v]
if v in self.edges:
    return self.edges[v]
raise IndexError(f"Vertex or edge {v} not found")
# or some more elaborated message, like
# "Couldn't find vertex {v} or edge connecting vertices {v[0]} and {v[1]}"
# when v is a tuple of 2 elements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add tuple key support in GenericGraph.__getitem__()

2 participants