manim icon indicating copy to clipboard operation
manim copied to clipboard

Add edge-key support in `GenericGraph.__getitem__()`

Open HairlessVillager opened this issue 1 year ago • 5 comments

Motivation and Explanation: Why and how do your changes improve the library?

In the old GenericGraph, you can get the vertex 1 via g[1], but you cannot get the edge 1->2 via g[(1, 2)]. I think this is a little confusing, so I add this support. See #3798.

Links to added or changed documentation pages

https://docs.manim.community/en/stable/reference/manim.mobject.graph.Graph.html#movingvertices: I saw the g[1] in this documentation. It's worth to consider add some examples (e.g. g[(1, 2)].animate.set_color(RED)).

Reviewer Checklist

  • [ ] The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • [ ] If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • [ ] If applicable: newly added functions and classes are tested

HairlessVillager avatar Jun 09 '24 15:06 HairlessVillager

I would appreciate it if you could add some examples to the documentation, so that people know how this works.

Great idea! I'll update the documentation later.

HairlessVillager avatar Jun 11 '24 06:06 HairlessVillager

At some point when I wrote the __getitem__-interface for graphs, I wanted to lean on the mathematical notation for induced subgraphs. In other words, I'd have liked to implement that for a graph G, the expression G[v1, v2, v3] should return the subgraph induced by the vertices v1, v2, v3.

However, given that G[v1] currently also returns a vertex, and not a Graph with a single vertex, I think I should abandon my original idea, the proposed access to edges is a good idea.

The current implementation needs to be improved: Vertices can also be tuples, it is not safe to assume that when a tuple is fed to the graph that it will correspond to an edge:

verts = [(i, j) for i in range(4) for j in range(4)]
edges = [((0, 0), (0, 1)), ((0, 0), (1, 0)), ((1, 1), (1, 0)), ((1, 1), (0, 1)), ((1, 1), (1, 2)), ((1, 1), (2, 1)), ...]
G = Graph(verts, edges)

behackl avatar Jun 11 '24 08:06 behackl

At some point when I wrote the __getitem__-interface for graphs, I wanted to lean on the mathematical notation for induced subgraphs. In other words, I'd have liked to implement that for a graph G, the expression G[v1, v2, v3] should return the subgraph induced by the vertices v1, v2, v3.

However, given that G[v1] currently also returns a vertex, and not a Graph with a single vertex, I think I should abandon my original idea, the proposed access to edges is a good idea.

As far as I know, the GenericGraph use networkx to logically represent a graph. Maybe you can use this for some operation of graphs like getting induced subgraphs.

The current implementation needs to be improved: Vertices can also be tuples, it is not safe to assume that when a tuple is fed to the graph that it will correspond to an edge...

Good question! And it's easy to think of plan A that using isinstance() to check it's a edge-key or vertex-key. But when unfortunately vertice is ugly like vertice = [1, 2, (1, 2)], it requires a also ugly code for check.

Plan B is to use in keyword. I mean:

  1. If k in self.vertice, then k is a vertex-key.
  2. Else if k in self.edges, then k is a edge-key.
  3. Else raise a KeyError

Step 1 and 2 can be swapped for real implementation. It's clean and transparent and I thought it's behavior can be expected.

I prefer plan B and I look forward to your suggestions.🤗

HairlessVillager avatar Jun 11 '24 09:06 HairlessVillager

I would prefer to let @behackl review this, as I haven't used graphs enough to look at the use cases :)

JasonGrace2282 avatar Jun 21 '24 13:06 JasonGrace2282

Hi @behackl, I'd like to invite you to review my fix. Feel free to comment🥰

HairlessVillager avatar Jun 29 '24 03:06 HairlessVillager