Skip to content
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

cmath.log has an invalid signature #89381

Open
mdickinson opened this issue Sep 16, 2021 · 17 comments
Open

cmath.log has an invalid signature #89381

mdickinson opened this issue Sep 16, 2021 · 17 comments
Labels
3.10 3.11 3.12 extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@mdickinson
Copy link
Member

mdickinson commented Sep 16, 2021

BPO 45218
Nosy @mdickinson, @vstinner, @corona10

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2021-09-16.09:09:00.708>
labels = ['extension-modules', 'type-bug', '3.9', '3.10', '3.11']
title = 'cmath.log has an invalid signature'
updated_at = <Date 2021-09-24.15:55:05.789>
user = 'https://github.com/mdickinson'

bugs.python.org fields:

activity = <Date 2021-09-24.15:55:05.789>
actor = 'corona10'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Extension Modules']
creation = <Date 2021-09-16.09:09:00.708>
creator = 'mark.dickinson'
dependencies = []
files = []
hgrepos = []
issue_num = 45218
keywords = []
message_count = 5.0
messages = ['401933', '401937', '401938', '401939', '401940']
nosy_count = 3.0
nosy_names = ['mark.dickinson', 'vstinner', 'corona10']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue45218'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

Linked PRs

@mdickinson
Copy link
Member Author

inspect.signature reports that the cmath.log function has an invalid signature:

Python 3.11.0a0 (heads/fix-44954:d0ea569eb5, Aug 19 2021, 14:59:04) [Clang 12.0.0 (clang-1200.0.32.29)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import cmath
>>> import inspect
>>> inspect.signature(cmath.log)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mdickinson/Python/cpython/Lib/inspect.py", line 3215, in signature
    return Signature.from_callable(obj, follow_wrapped=follow_wrapped,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mdickinson/Python/cpython/Lib/inspect.py", line 2963, in from_callable
    return _signature_from_callable(obj, sigcls=cls,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mdickinson/Python/cpython/Lib/inspect.py", line 2432, in _signature_from_callable
    return _signature_from_builtin(sigcls, obj,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mdickinson/Python/cpython/Lib/inspect.py", line 2244, in _signature_from_builtin
    return _signature_fromstr(cls, func, s, skip_bound_arg)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mdickinson/Python/cpython/Lib/inspect.py", line 2114, in _signature_fromstr
    raise ValueError("{!r} builtin has invalid signature".format(obj))
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: <built-in function log> builtin has invalid signature

@mdickinson mdickinson added 3.9 3.10 3.11 extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error labels Sep 16, 2021
@mdickinson
Copy link
Member Author

Issue bpo-43067 is similar. I'm not sure what the best solution is in this case:

  • un-argument-clinic cmath.log, and document the signature using two lines (similar to range):
     log(z)
     log(z, base)
  • change the behaviour of cmath.log so that the second argument is allowed be None (and defaults to None)

@mdickinson
Copy link
Member Author

See also bpo-36306 and bpo-29299. There may be nothing to be done here, but it would be nice if math.log and cmath.log at least behaved in the same way. A default of None doesn't seem like a terrible option.

(BTW, just to forestall the suggestion, a default of math.e or cmath.e is definitely not the right thing to use as a default: math.e is not exactly Euler's value for e, and log base math.e is likely to be less accurate than plain natural log.)

@mdickinson
Copy link
Member Author

and log base math.e is likely to be less accurate than plain natural log

Nope, that's nonsense, since the two-argument form simply divides by log(base), and while log(math.e) is mathematically not exactly 1 (its exact value, assuming IEEE 754 binary64, starts 0.9999999999999999468176229339410862948..., which is off from 1.0 by around 0.48 ulps), it's *just* close enough that with a well-behaved libm log implementation it's exactly 1.0 after rounding.

I still don't like the idea of math.e as a default value here, but I'd have to work harder to identify why it makes me uneasy.

@vstinner
Copy link
Member

The current Argument Clinic syntax for math.log() is:

--------------
/*[clinic input]
math.log

x:    object
[
base: object(c_default="NULL") = math.e
]
/

Return the logarithm of x to the given base.
(...)
--------------

math.log.__text_signature__ is None. __text_signature__ is used by inspect.signature().

I guess that it's a bug in Argument Clinic.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@skirpichev
Copy link
Contributor

The underlying reason of failure for this issue is that the math.e (default value) is neither in the module dict (i.e. cmath) nor in the sys_module_dict. (Hence wrap_value() in the inspect module fails.) I don't see a clear solution here.

#101070 implements a workaround with base=None defaults (both for math and cmath), as it was suggested above (I hope) by @mdickinson.

@mdickinson mdickinson added 3.12 and removed 3.9 labels Jan 16, 2023
@hauntsaninja
Copy link
Contributor

hauntsaninja commented Jan 18, 2023

I'm not sure the failure is to do with module dict.

Like Victor mentions, I think there's some bug or missing feature in Argument Clinic. If you manually modify the generated file to:

diff --git a/Modules/clinic/mathmodule.c.h b/Modules/clinic/mathmodule.c.h
index 9fac1037e5..4c463a49b0 100644
--- a/Modules/clinic/mathmodule.c.h
+++ b/Modules/clinic/mathmodule.c.h
@@ -187,7 +187,7 @@ exit:
 }
 
 PyDoc_STRVAR(math_log__doc__,
-"log(x, [base=math.e])\n"
+"log(x, base=math.e)\n--\n\n"
 "Return the logarithm of x to the given base.\n"
 "\n"
 "If the base not specified, returns the natural logarithm (base e) of x.");

Then __text_signature__ and inspect.signature do the right thing.

Edit: what skirpichev says is true of cmath, but not for math. For cmath we can just use cmath.e

@hauntsaninja
Copy link
Contributor

hauntsaninja commented Jan 18, 2023

Actually, looking at it again, why do we need the optional group in mathmodule.c at all? Maybe I'm missing something simple, but removing the optional group should just make all the signature stuff work. I'll open a PR and you can tell me what I'm missing...

hauntsaninja added a commit to hauntsaninja/cpython that referenced this issue 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 doing, which is use c_default = NULL
(so the actual runtime logic is unchanged), but mark cmath.e as the fake
default value.
hauntsaninja added a commit to hauntsaninja/cpython that referenced this issue 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 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

After some debugging, it seems that the mentioned by @vstinner math.log issue is not a Clinic's bug, rather a feature. Here is a comment from the clinic.py:

# docstring_only means "don't generate a machine-readable
# signature, just a normal docstring".  it's True for
# functions with optional groups because we can't represent
# those accurately with inspect.Signature in 3.4.

Correct me, but it's not something that is planned to be in the Signature someday, right? Some searching suggests me that there are instead plans to convert such cases to arg=default notation (see this).

@hauntsaninja
Copy link
Contributor

Yes, that's correct. The thing that was confusing for me here is that an optional group is seemingly unnecessary for math.log — so when I posted my first comment I didn't even realise there was one!
See e.g. curses.window.addch for an example of something that can't be represented accurately with an inspect.Signature https://docs.python.org/3/library/curses.html#curses.window.addch

@skirpichev
Copy link
Contributor

BTW, it seems the broken signature is not a bug for stdlib. See #101123.

@fochoao
Copy link

fochoao commented Jan 19, 2023

cmath.log has an invalid signature.

@fochoao
Copy link

fochoao commented Jan 19, 2023

5282f4f indeed, solved.

@rhettinger
Copy link
Contributor

log(x, base=None, /)
    Return the logarithm of x to the given base.

    If the base is not specified or is None, returns the natural
    logarithm (base e) of x.

FWIW, I object to these changes. They are changing the user experience negatively. It is incorrect to say these were "invalid". The functions predated signature objects. They aren't broken; rather, they are like the range() function in that signatures have not yet grown the ability the express what they do (in contrast, type annotations support overloads and have no problem with these functions).

In Python 3.11, the doc entry read log(x, [base=math.e]) which was succinct helpful and correct. Now, the "signature" has changed. The log function never took a None argument — it stayed firmly in the world of floats as does it C counterpart. Now, the help and tooltips are less useful. Nothing about log(x, base=None) communicates that a natural log is the default.

I haven't done a new speed measurement but presumably having to check for None before calling the float conversion is more expensive than just directly converting to a float when a second argument is present. This is problematic in fine grained functions like log that are supposed to be short, fast, and suitable for use in a loop.

AFAICT, no actual user problem is being solved. No one have ever reported a usability issue here. Instead, the focus seems to be on shoe-horning functions into the argument clinic and signature arguments. When they two don't match (as in this case), there seems to be willingness to damage the function rather than on improving the capabilities of signature objects.

Summary: I don't think log should take None as a argument. AFAICT no other math tool or other programming language does this. It is a strictly float/complex argument. We really don't want to allow or encourage users to write log(x, None). That is a usability regression. Likewise, the new tooltip and doc entry are less usable. While people are all about everything having a signature object might be made happier, actual users of math.log are worse off.

This is permanent, forever API change. I don't think we have to live with the likes of log(12.34, None) in perpetuity.

@rhettinger
Copy link
Contributor

I've attached a screenshot of what the tooltips now look like in IDLE. Note, there is no mention of a natural logarithm being the default.

Screenshot 2023-01-29 at 4 01 30 PM

I would like to see these changes reverted.

@hauntsaninja
Copy link
Contributor

hauntsaninja commented Jan 29, 2023

@rhettinger Apologies if I merged it too quickly!

I'm curious if you feel the other PR is useful: #101115
That PR has no effect on the implementation of math.log or cmath.log, other than simplifying the generated argument parsing code for math.log (edit: which should result in improved perf).

re: showing the base explicitly, @mdickinson expressed some concern about about implying that math.log(x) is equivalent to math.log(x, math.e) in #101115 (comment) and above.

@skirpichev
Copy link
Contributor

skirpichev commented Jan 30, 2023

About benchmarks.

On the main (666c084):

$ ./python -m timeit -s 'from math import log' -s 'n = 10' \
    -s 'a = [1.1 for _ in range(n)]' '[log(_) for _ in a]'
50000 loops, best of 5: 5.17 usec per loop
$ ./python -m timeit -s 'from math import log' -s 'n = 100' \
    -s 'a = [1.1 for _ in range(n)]' '[log(_) for _ in a]'
5000 loops, best of 5: 41.9 usec per loop
$ ./python -m timeit -s 'from math import log' -s 'n = 1000' \
    -s 'a = [1.1 for _ in range(n)]' '[log(_) for _ in a]'
1000 loops, best of 5: 400 usec per loop
$ ./python -m timeit -s 'from cmath import log' -s 'n = 10' \
    -s 'a = [1.1 + 1.2j for _ in range(n)]' '[log(_) for _ in a]'
50000 loops, best of 5: 5.25 usec per loop
$ ./python -m timeit -s 'from cmath import log' -s 'n = 100' \
    -s 'a = [1.1 + 1.2j for _ in range(n)]' '[log(_) for _ in a]'
5000 loops, best of 5: 41.8 usec per loop
$ ./python -m timeit -s 'from cmath import log' -s 'n = 1000' \
    -s 'a = [1.1 + 1.2j for _ in range(n)]' '[log(_) for _ in a]'
1000 loops, best of 5: 387 usec per loop

On the branch (was rebased wrt main):

$ ./python -m timeit -s 'from math import log' -s 'n = 10' \
    -s 'a = [1.1 for _ in range(n)]' '[log(_) for _ in a]'
100000 loops, best of 5: 2.85 usec per loop
$ ./python -m timeit -s 'from math import log' -s 'n = 100' \
    -s 'a = [1.1 for _ in range(n)]' '[log(_) for _ in a]'
10000 loops, best of 5: 21.1 usec per loop
$ ./python -m timeit -s 'from math import log' -s 'n = 1000' \
    -s 'a = [1.1 for _ in range(n)]' '[log(_) for _ in a]'
2000 loops, best of 5: 196 usec per loop
$ ./python -m timeit -s 'from cmath import log' -s 'n = 10' \
    -s 'a = [1.1 + 1.2j for _ in range(n)]' '[log(_) for _ in a]'
50000 loops, best of 5: 5.29 usec per loop
$ ./python -m timeit -s 'from cmath import log' -s 'n = 100' \
    -s 'a = [1.1 + 1.2j for _ in range(n)]' '[log(_) for _ in a]'
5000 loops, best of 5: 41.9 usec per loop
$ ./python -m timeit -s 'from cmath import log' -s 'n = 1000' \
    -s 'a = [1.1 + 1.2j for _ in range(n)]' '[log(_) for _ in a]'
1000 loops, best of 5: 389 usec per loop

Indeed, there is slight speed regression for the cmath.log. But for the math.log, there is a speed gain...

rather than on improving the capabilities of signature objects.

@rhettinger, that you would like here, as I guess, is the #101115, but with a symbolic constant in the signature/help instead, correct?

Actually, this might work like this (same for the math module):

diff --git a/Modules/cmathmodule.c b/Modules/cmathmodule.c
index 2038ac26e6..861a824d77 100644
--- a/Modules/cmathmodule.c
+++ b/Modules/cmathmodule.c
@@ -952,7 +952,7 @@ cmath_tanh_impl(PyObject *module, Py_complex z)
 cmath.log
 
     z as x: Py_complex
-    base as y_obj: object = NULL
+    base as y_obj: object(c_default="NULL") = e
     /
 
 log(z[, base]) -> the logarithm of z to the given base.

With that change we have:

>>> import cmath
>>> cmath.log.__text_signature__
'($module, z, base=e, /)'

But, unfortunately (and that's coming from the inspect module logic in RewriteSymbolics):

>>> help(cmath.log)
Help on built-in function log in module cmath:

log(z, base=2.718281828459045, /)
    log(z[, base]) -> the logarithm of z to the given base.

    If the base not specified, returns the natural logarithm (base e) of z.

Something like this is possible with a tentative patch attached (with work with base=cmath.e too):

>>> inspect.signature(cmath.log)
<Signature (z, base=e, /)>
>>> help(cmath.log)
Help on built-in function log in module cmath:

log(z, base=e, /)
    log(z[, base]) -> the logarithm of z to the given base.

    If the base not specified, returns the natural logarithm (base e) of z.

inspect-sym-defs.diff.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.10 3.11 3.12 extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

6 participants