Skip to content

[EXPERIMENTAL] Fix some graph tests#4569

Open
fmuenkel wants to merge 5 commits intoManimCommunity:experimentalfrom
fmuenkel:fix_graph_tests
Open

[EXPERIMENTAL] Fix some graph tests#4569
fmuenkel wants to merge 5 commits intoManimCommunity:experimentalfrom
fmuenkel:fix_graph_tests

Conversation

@fmuenkel
Copy link
Contributor

@fmuenkel fmuenkel commented Feb 3, 2026

Overview: What does this pull request change?

Currently a lot of tests in test_graph.py fail, because __str__() is missing from Graph and DiGraph.
This PR

  • adds __str__() wrapping around __repr__()
  • uses numpy's assert_allclose() to make two more test pass
  • fixes a bug in graph.py's GenericGraph._add_vertices_animation() where not all temporary vertices were removed from the scenebuffer after the animation.

Now all tests in test_graph.py pass.

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

Links to added or changed documentation pages

Further Information and Comments

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

@fmuenkel fmuenkel marked this pull request as ready for review February 4, 2026 09:13
Copy link
Contributor

@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! I left some comments:

Comment on lines 915 to 917
if buf is not None and hasattr(buf, "replace"):
with contextlib.suppress(Exception):
buf.replace(v[-1])
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't been able to test this locally yet, but:

  • Is the hasattr(buf, "replace") check necessary? If buf is a SceneBuffer, it must already have a replace() method...
  • Why is replace() used instead of remove()?
  • What exceptions might happen by using this? Is it OK to suppress them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Yes, I believe hasattr is redundant here.
  • So, this took me a while. remove() did not work - it still left Dot and Line mobjects in scene.mobjects. I just happen to try replace() and it worked, but honestly, I don't understand the new rendering process well enough yet to figure out why.
  • I was thinking of cases like dry-run testing or if one of the mobjects was already removed. We don't want the run to fail, since the vertex was already created.

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.

2 participants