Skip to content

gh-89381: Fix signature for math.log and cmath.log #101115

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

Closed
wants to merge 2 commits into from

Conversation

hauntsaninja
Copy link
Contributor

@hauntsaninja hauntsaninja commented Jan 18, 2023

The optional group in math.log seems unnecessary, as far as I can tell. Once we remove this, Argument Clinic knows what to do.

For cmath, we just do what math is already doing: use c_default = NULL (so the actual runtime logic is unchanged), but mark cmath.e as the fake default value.

The optional group in math.log seems unnecessary, as far as I can tell.
Once we remove this, Argument Clinic knows what to do.

For cmath, we just do what math is doing, which is use c_default = NULL
(so the actual runtime logic is unchanged), but mark cmath.e as the fake
default value.
@skirpichev
Copy link
Contributor

Seems to be working. I'll keep #101070 in case someone will prefer base=None defaults. Now we got
something like this:

>>> inspect.signature(math.log)
<Signature (x, base=2.718281828459045, /)>

Maybe this is slightly cryptic: 1) magic constant appear, 2) there is an illusion that the special base=math.e works internally as log(x)/log(base). BTW, shouldn't be there regression tests (e.g. for the original issue with inspect.signature(cmath.log))?

Also, I think that docs for the math.log/cmath.log should be adapted. To me it was a surprise, that this requires a manual adjustment after fixing the docstring.

@mdickinson mdickinson self-requested a review January 19, 2023 16:28
@mdickinson
Copy link
Member

Maybe this is slightly cryptic: 1) magic constant appear, 2) there is an illusion that the special base=math.e works internally as log(x)/log(base).

Agreed on both points; I prefer the solution in #101070.

@skirpichev
Copy link
Contributor

@mdickinson, I can restore #101070, if you wish. But after #101123 I'm not sure that a broken signature is an issue for CPython.

@mdickinson
Copy link
Member

@skirpichev The changes in #101070 look good to me, modulo one documentation suggestion (left in a comment). If you resurrect it, I'd be happy to merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants