Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-131885: simplify function signatures in the cmath module#131886
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
In the main:$ ./python -m timeit -r11 -s 'from cmath import sin;z=1j' 'sin(z)'1000000 loops, best of 11: 312 nsec per loopWith patch:$ ./python -m timeit -r11 -s 'from cmath import sin;z=1j' 'sin(z)'1000000 loops, best of 11: 330 nsec per loop
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Now function docstrings are in sync with the module docs, for example:```pycon>>> import cmath, inspect>>> help(cmath.sin)Help on built-in function sin in module cmath:sin(z) Return the sine of z.>>> inspect.signature(cmath.sin)<Signature (z)>```As a side effect, this allows `sin(z=1.23)` calls. The price is slightperformance penalty (maybe this can be fixed on AC side):| Benchmark | ref | patch ||----------------|:------:|:--------------------:|| sin(1.1) | 417 ns | 428 ns: 1.03x slower || sin(1j) | 414 ns | 422 ns: 1.02x slower || log(2.3) | 390 ns | 409 ns: 1.05x slower || log(1+2j) | 449 ns | 468 ns: 1.04x slower || log(2.1, 3.2) | 601 ns | 630 ns: 1.05x slower || Geometric mean | (ref) | 1.04x slower |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What's the justification for these runtime changes? I don't see any value to being able to usez as a keyword argument, and we shouldn't make things slower just for aesthetics (removing the/ inhelp()).
It's explained in the pr description and (in depth) issue thread. With this change we have correct signatures in docs, without cluttering them with syntax, which might be hard for newbies.
No, it's not for aesthetics, but for readability. In pure-Python world you have toadd extra syntax to have effect of positional-only arguments. This is something people have to learn and it's not common for being in recipes, tutorials, etc. IMO, trade speed for readability is good. Though, maybe not for all cases (e.g. |
The signatures in the documentation are already correct, no? The debate over slash and star is over how precisely we describe runtime semantics. Is there something else I'm missing? The documentation should be driven by the runtime, not vice versa. A |
No. As for many modules, the cmath sphinx docs are out of sync with module docstrings. Alternative to this pr will be adding trailing slashes to sphinx docs.
I think that documentation should be 1) readable 2) accurately describe runtime behavior. To achieve 1) and 2) we can adjust either docs or runtime, or both. |
To be clear though, the only difference is the slash? I wouldn't describe that as 'wrong', but as 'slightly less precise'. On the assumption that the above is true, I'll close this PR. Perhaps there is a JavaScript solution we can use to 'simplify' the documentation for beginners. A |
@AA-Turner, we have EBdecision: "If a function accepts positional-only or keyword-only arguments, include the slash and the star in the signature as appropriate: [...] Although the syntax is terse, it is precise about the allowable ways to call the function and is taken from Python itself." Sorry, I don't see here option for compromises.
Currently stars and slashes are highlighted and tooltips appear in the sphinx docs. Though, same syntax appears e.g. in the help() output without any hints for user... |
BTW, can you change your mind if speed issue will be solved? |
Uh oh!
There was an error while loading.Please reload this page.
Now function docstrings are in sync with the module docs, for example:
As a side effect, this allows
sin(z=1.23)calls. The price is slightperformance penalty (maybe this can be fixed on AC side):
*and/as needed #131885For reviewers:
benchmark code