FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-89381: Fix invalid signatures of math/cmath.log (v2) by skirpichev · Pull Request #101593 · python/cpython · GitHub

Repository navigation

gh-89381: Fix invalid signatures of math/cmath.log (v2) - #101593

Closed
skirpichev wants to merge 4 commits into
python:mainfrom
skirpichev:fix-89381
Closed

skirpichev wants to merge 4 commits into
python:mainfrom
skirpichev:fix-89381

Conversation

skirpichev commented Feb 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Restored 0ef92d9 after reversion in #101580 (1st commit). Here is discussion opened:
https://discuss.python.org/t/should-none-defaults-for-optional-arguments-be-discouraged/23554

To solve some mentioned minor issues (on top of base=None approach):

Some benchmarks.

On the main:

$ ./python -m timeit -s 'from math import log' 'log(1.1)'
1000000 loops, best of 5: 338 nsec per loop
$ ./python -m timeit -s 'from cmath import log' 'log(1.1)'
1000000 loops, best of 5: 298 nsec per loop

With first commit:

$ ./python -m timeit -s 'from math import log' 'log(1.1)'
2000000 loops, best of 5: 165 nsec per loop
$ ./python -m timeit -s 'from cmath import log' 'log(1.1)'
1000000 loops, best of 5: 299 nsec per loop

I would guess, that some positive difference for the math.log - due to the AC difference.

stevendaprano commented Feb 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Are we really going to accept an explicit argument None for the default base? I'm not sure what I think of that.

Edit: to be clear, I see that it is convenient to be able to pass an explicit argument to mean 'give me the default', but log(x, base=None) reads wrong to me. We're doing a logarithm without a base?

Copy link
Copy Markdown
Member Author

Are we really going to accept an explicit argument None for the default base?

Not yet (see the issue and mentioned discussion). That's why this is a draft.

it is convenient to be able to pass an explicit argument to mean 'give me the default'

Unfortunately, we can't pass other default in this case (no such entity as the Euler's number in the stdlib). Before it was base=math.e, but that's wrong: with the default value this functions returns the natural logarithm of x, not log(x)/log(math.e).

We're doing a logarithm without a base?

Yes, without this argument it's assumed to be the natural logarithm of x. The difference is just in the log function signature, it was before log(x[, base]) - not that currently the inspect.Signature does support.

Copy link
Copy Markdown
Member

We don't have core dev consensus on this: @rhettinger in particular is strongly opposed to the introduction of None here. Let's close on the "status quo wins a stalemate" basis.

mdickinson closed this Mar 4, 2023
skirpichev deleted the fix-89381 branch March 4, 2023 08:47

Copy link
Copy Markdown
Member Author

@mdickinson, I appreciate your judgment here, but unless I miss that, the perm() function case was just ignored by @rhettinger (both in the issue and in the discussion thread). Same module, the default value was introduced by... Is that was an error, or?

I see point to make the inspect module/AC more expressive to describe de-facto existing syntax (multiple signatures), that has no pure-Python counterpart yet. That clearly does make sense for more complex cases (e.g. range). But does it make sense in the given example? I'm not sure that it was shown in the discussion. Unfortunately, there are no other options suggested (c.f. multiple signatures) to solve the issue, e.g. I doubt it's possible to use base=math.e default & provide correct inspect.Signature's.

And there is another, yet related, issue: log() is slow (see pr description) just to keep the "helpful and correct" "signature". Maybe it does make sense to revert the AC stuff here to workaround this?

Copy link
Copy Markdown
Member

@skirpichev I understand (and share) your frustration, but I don't think anything's going to be achieved by further discussion here. It's not that big a deal, and it's time to let it go.

Copy link
Copy Markdown
Member Author

@mdickinson, ok. But you closed the issue as well. That sounds (at least for me): "we won't any progress here at all" (that includes Raymond's suggestion). If this somehow was related to my personal involvement in the discussion - please reconsider: I can promise to be outside of any further discussions and/or any touching of math/cmath code, whatever you feel better.

What I actually want - some public interface, that will allow me to add introspection capabilities to external extensions. Personally I'm satisfied with the current "state of art" (except for #101872 maybe), but now I see why all this stuff (e.g. __signature__ or __text_signature__ attributes) can't be made public yet. This bug is an example. Don't hide problems.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL