Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replaced current tree layout algorithm with SageMath's for improved layout of large trees #2343

Merged
merged 3 commits into from Dec 2, 2021

Conversation

behackl
Copy link
Member

@behackl behackl commented Nov 30, 2021

Overview: What does this pull request change?

See title, and the following example for trees with 100 vertices:

Current main branch:
UncoverTree_ManimCE_v0 11 0

This PR:
UncoverTree_ManimCE_v0 11 0

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

@behackl behackl added the enhancement Additions and improvements in general label Nov 30, 2021
@hydrobeam hydrobeam changed the title Replaced current tree layout algorithm by SageMath's one for improved layout of large trees Replaced current tree layout algorithm with SageMath's for improved layout of large trees. Nov 30, 2021
Copy link
Member

@hydrobeam hydrobeam left a comment

Choose a reason for hiding this comment

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

LGTM, left a minor comment about a parameter of the method.

) -> dict:
result = {root_vertex: np.array([0, 0, 0])}
scale: Union[float, tuple] = 2,
orientation: str = "down",
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way to influence this parameter? Maybe borrowing sagemath's docs for this param + adding a test / a way to access it when creating the Graph would be nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically yes -- however, I don't think this is very useful and the parameter is just kept here so that I don't have to refactor the remaining code. In SageMath, interacting with the plot is much more difficult than in Manim, where reversing the tree orientation is simply a matter of vertically flipping the resulting Mobject.

@behackl behackl enabled auto-merge (squash) December 2, 2021 13:41
@behackl behackl merged commit f40ba6a into ManimCommunity:main Dec 2, 2021
@behackl behackl changed the title Replaced current tree layout algorithm with SageMath's for improved layout of large trees. Replaced current tree layout algorithm with SageMath's for improved layout of large trees Dec 3, 2021
@behackl behackl deleted the improved-tree-layout branch May 16, 2022 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Additions and improvements in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants