Skip to content

Conversation

@chopan050
Copy link
Contributor

@chopan050 chopan050 commented Jul 13, 2023

Overview: What does this pull request change?

As the title says, I optimized Axes.coords_to_point. Best results if combined with PR #3285: Optimized :meth:NumberLine.number_to_point.

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

  • Mainly, I got rid of the np.sum and the list comprehension, and replaced them with a good ol' for loop, which is much more efficient in this case because there are only 2 or 3 coords to add.
  • I avoided using VMobject.__iter__ on self.axes by iterating directly over its submobjects attribute.
  • Also, I removed one unnecessary addition and subtraction of origin by assigning first the result of self.x_axis.number_to_point(coords[0]) by separate (which should not be a problem since x_axis is already being used explicitly to calculate origin.
  • Finally, in general I worked to make the code more readable.

The changes I propose:

    def coords_to_point(
        self, *coords: float | Sequence[float] | Sequence[Sequence[float]] | np.ndarray
    ) -> np.ndarray:
        coords = np.asarray(coords)
        origin = self.x_axis.number_to_point(
            self._origin_shift([self.x_axis.x_min, self.x_axis.x_max]),
        )

        # Is every component in coords in the format [xi, yi, zi]?
        # i.e. is coords in the format ([[x1 y1 z1] [x2 y2 z2] ...])? (True)
        #
        # Or is coords in the format (x, y, z) or ([x1 x2 ...], [y1 y2 ...], [z1 z2 ...])? (False)
        #
        # The latter is preferred.
        are_components_xyz = False

        # If coords is in the format ([[x1 y1 z1] [x2 y2 z2] ...]):
        if coords.ndim == 3:
            # Extract from original tuple: now the format is [[x1 y1 z1] [x2 y2 z2]]
            coords = coords[0]

            # If there's a single coord, extract it so that coords_to_point returns a single point
            if coords.shape[0] == 1:
                coords = coords[0]  # In this case, now coords = [x1 y1 z1]
            else:
                are_components_xyz = True
                # Transform coords into the format [[x1 x2 ...] [y1 y2 ...] [z1 z2 ...]]
                # for later processing.
                coords = coords.T

        # Now coords should be in the format [x y z], where each component is either a float or an ndarray
        points = self.x_axis.number_to_point(coords[0])
        other_axes = self.axes.submobjects[1:]
        for axis, nums in zip(other_axes, coords[1:]):
            points += axis.number_to_point(nums) - origin

        if are_components_xyz:
            return points
        return points.T  # Has no effect on a 1D array representing a single point

The results before and after the changes (using the same test scene I used for #3285). As you can see, I managed to cutoff around 0.70 seconds from Axes.coords_to_point. If the changes in the previous PR get accepted as well, there's even more speedup:

Before After
imagen imagen

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

@chopan050 chopan050 changed the title Optimized :meth:Axes.coords_to_point Optimized :meth:.Axes.coords_to_point Jul 15, 2023
@behackl behackl self-requested a review July 16, 2023 08:21
Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

One minor suggestion, otherwise this is a fine improvement. Please take a look & thanks for your efforts!

# Or is coords in the format (x, y, z) or ([x1 x2 ...], [y1 y2 ...], [z1 z2 ...])? (False)
#
# The latter is preferred.
are_components_xyz = False
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be made even more explicit: are_components_xyz is not particularly self-explanatory. Why not are_coordinates_transposed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I like that one better. The original transposed variable was not really clear IMO, but in hindsight are_components_xyz is not much better.

Gonna change that!

@chopan050
Copy link
Contributor Author

Done! I changed the variable name to are_coordinates_transposed, I liked that one better.

I also added some more commentaries and added an extra condition points.ndim == 1 to the final return part. It bothered me a bit that the "single point" case falled into the return points.T situation and wanted to make more explicit that the single point should also be returned as is, but I forgot to add that before.

Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

LGTM, thank you very much!

@behackl behackl merged commit e00c780 into ManimCommunity:main Jul 16, 2023
@behackl behackl added maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements performance labels Jul 16, 2023
@behackl behackl added this to the v0.18.0 milestone Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants