Skip to content

Enhance throw_error_if_no_points error message - #4927

Open
smmariquit wants to merge 2 commits into
ManimCommunity:mainfrom
smmariquit:fix-throw-error-if-no-points
Open

Enhance throw_error_if_no_points error message#4927
smmariquit wants to merge 2 commits into
ManimCommunity:mainfrom
smmariquit:fix-throw-error-if-no-points

Conversation

@smmariquit

Copy link
Copy Markdown

Resolves #4921. Replaces the generic Exception with a ValueError and explicitly names the class and points out if submobjects contain points. This is particularly helpful when a VGroup is accidentally passed, as the new error message clearly identifies it as a container with no points of its own.

@behackl behackl left a comment

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.

Thanks for your contribution! There is a few small things I'd like to discuss before we get this merged.

  1. In the case where there is only one child the error reads "..., but 1 of its 1 submobjects do", which reads a bit weird. I'd be in favour of special-casing the 1-child situation.
  2. Checking direct children also means that nested mobjects like VGroup(VGroup(Dot())) would not get a more helpful message. The question (that I don't really have a great answer to, would like to hear your opinion) is whether restricting the check to direct children is fine, or whether iterating over the entire family of a mobject would make more sense?
  3. Specifying the raised exception is a good idea too, I'm not sure about ValueError though. Isn't this more of a RuntimeError situation?
  4. Tests please! I think there is a test for the old version of this exception already, this should be extended a bit to reflect the more detailed exception text.

Thanks again, let me know what you think!

@nikolajmunk

nikolajmunk commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

My $0.02:

  • I'm not personally convinced that mentioning submobjects in this error message is always beneficial. Consider a case where a mobject with children has its points mistakenly removed - here, the helpful "did you mean to..." hint might actually bring the user (especially an inexperienced one) further from the source of the problem. (More generally I'm not convinced that "user actually meant to call this on a child of mob, not mob itself" is a common enough problem to special-case in an error message that covers other cases as well, but I'm fully willing to accept if I'm out of touch here!)
  • I think it would make sense to include {self} at least somewhere in the f-string, since this permits the error message to use any special names that are defined via self.name or self.__repr__ (for example, a text mobject would show up as Text("ABC")) - plus it provides more information about VGroup children.
  • IMO ValueError is correct here: the user is passing an object of the correct type but with some invalid property. In my mind it's analogous to calling sqrt(x) in a script where x was erroneously decremented to -1 (or where we actually meant to pass a positive y). A custom InvalidMobjectError type might also make sense down the line, I'm sure people could find other use cases for it.

@nikolajmunk

Copy link
Copy Markdown
Contributor

To challenge my own previous comment: Perhaps the "did you mean to pass..." message actually makes perfect sense if the method is explicitly called on a group-like mobject (Group, VGroup, VDict)?

It is very uncommon for people to manually add points to group mobjects, so if such a mobject is passed to a point-requiring method, it's a good indication that something unintended is going on.

This doesn't catch VDict, but maybe something like this?:

message = f"Cannot call {cls}.{caller_name} because {self} has no points."
if (
    isinstance(self, self.get_group_class())
    and (drawn := [m for m in self if m.has_points()])
):
    message += (
        f" It has {len(drawn)} submobjects with points, "
        "did you mean to pass one of them?"
    )

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.

throw_error_if_no_points reports Mobject for any subclass, and never says which object

3 participants