Skip to content

Fix unused rate_func parameter with Unwrite#4732

Open
Ayowel wants to merge 1 commit into
ManimCommunity:mainfrom
Ayowel:fix/unwrite-rate-func
Open

Fix unused rate_func parameter with Unwrite#4732
Ayowel wants to merge 1 commit into
ManimCommunity:mainfrom
Ayowel:fix/unwrite-rate-func

Conversation

@Ayowel

@Ayowel Ayowel commented May 17, 2026

Copy link
Copy Markdown

Overview

Fix an issue that causes the rate_func function passed to Unwrite to be ignored.

Further Information and Comments

I chose to simply remove the rate_func parameter so it is passed down to the Write super (with the same default value) instead of repeating the unused parameter in the call to __init__.

This appears to be a single-occurence issue, a quick look through other creation classes does not show this error again.

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

@GoThrones

Copy link
Copy Markdown
Contributor

Keeping the explicit rate_func parameter in init is better.

The problem with your way of deleting it from init and making it pass through kwargs, is:
(1) No type hint — type checker can't validate what's passed
(2) No default value visible in the signature — user has to dig into Write or Animation to know the default
(3) No IDE autocomplete for rate_func
(4) A typo like rate_fnc=linear silently passes into **kwargs and gets ignored with no error.

But, if you keep it explicit in init and write an argument rate_func = rate_func in super(), then the benefits are:
(1) Type hint would be visible.
(2) Default value would be visible: linear
(3) IDE shows it in autocomplete.
(4) Typo rate_fnc=linear raises TypeError: unexpected keyword argument immediately.

The explicit parameter is essentially documentation that also enforces correctness. **kwargs is a kind of black hole. anything goes in, no validation, no visibility.

So, effectively, i would suggest you to not delete it form init, but keep it there, and in super(), write another parameter: rate_func = rate_func.

@Ayowel

Ayowel commented May 17, 2026

Copy link
Copy Markdown
Author

The problem with your way of deleting it from init and making it pass through kwargs, is: (1) No type hint — type checker can't validate what's passed (2) No default value visible in the signature — user has to dig into Write or Animation to know the default (3) No IDE autocomplete for rate_func (4) A typo like rate_fnc=linear silently passes into **kwargs and gets ignored with no error.

This looks like an AI answer, so I'm not sure if answering is of any use :/ .
Is your answer the community's stance or your own ? If it is the former, is there documentation I can refer to for code styling later ? I did not find anything on this subject.

As far as I can see, the function already uses kwargs, many other parameters are currently passed in the same way, numerous other Animation subclasses that can use rate_func do not redeclare it in their __init__, and modern IDEs do bubble-up the args signatures (or at least mine does, I don't know what you use), so:

  • (1) No type hint => Why should this specific parameter get preferential treatment over the many others that are passed down through kwargs ?
  • (2) No default value visible in the signature => Again, why should this parameter get preferential treatment ?
  • (3) No IDE autocomplete for rate_func => Yes there is.
  • (4) A typo like rate_fnc=linear silently passes into **kwargs and gets ignored with no error. => Irrelevant, this behavior is not a change introduced by this PR.

Additionally, if your intent in the long run is to add all possible parameters to function signatures to ensure that the "advantages" you enumerated, this would introduce a lot of code redundancy, would likely be error-prone, and be a pain to maintain should new parameters be introduced or existing parameters be changed/removed.

@GoThrones

GoThrones commented May 17, 2026

Copy link
Copy Markdown
Contributor

This looks like an AI answer, so I'm not sure if answering is of any use :/ .

The top 2 points(except the one about IDE autocomplete and about typo) were my own opinion, but then i asked an AI, in the hope of knowing more about it. But then, the AI suggested about that IDE autocomplete and that typo remark and that seemed valid point too. So, I added them too.

Is your answer the community's stance or your own ?

That's my own opinion. Although, if you need/want, you can check the contributing section here: https://docs.manim.community/en/stable/.

  • (1) No type hint => Why should this specific parameter get preferential treatment over the many others that are passed down through kwargs ?

Because rate_func is one of the most commonly used parameters when using such animations. A user shouldn't have to dig through Write, then Animation, just to find out they can pass rate_func to Unwrite. Keeping it explicit makes the class immediately usable without that extra research.

  • (2) No default value visible in the signature => Again, why should this parameter get preferential treatment ?

Same reason as above.

  • No IDE autocomplete for rate_func

I use VSCode. In that, it doesn't show, unless the parameter, it's typehint and default value is explicitly declared in init.

  • A typo like rate_fnc=linear silently passes into **kwargs and gets ignored with no error. => Irrelevant, this behavior is not a change introduced by this PR.

Actually, that is relevant. When rate_func is in the signature, a typo like rate_fun = linear would raise a TypeError saying: init got an unexpected keyword argument rate_fun. But if rate_func is removed from the signature, and is passed through kwargs, then that typo passes through without giving any error. The explicit signature acts as a validation layer that passing it through kwargs doesn't provide. And the important question is whether the explicit signature is better, not whether the PR introduced the problem.

However, I would wait to see what the other devs have to say.

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