Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-119594: Improve pow(fraction.Fraction(), b, modulo) error message#119593

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

Merged
terryjreedy merged 3 commits intopython:mainfromwimglenn:pow3-fractions
May 29, 2024

Conversation

@wimglenn
Copy link
Contributor

@wimglennwimglenn commentedMay 27, 2024
edited
Loading

Fraction.__pow__ does not accept the third argument whichpow tries to pass to it:

>>> pow(Fraction(1), 1, 1)Traceback (most recent call last):  File "<stdin>", line 1, in <module>TypeError: Fraction.__pow__() takes 2 positional arguments but 3 were given

Other number types don't do that, they support it when they can and print an error message if they don't want to, e.g.

>>> pow(Decimal("1"), 1, 1)Decimal('0')>>> pow(Decimal('1'), 1.1, 1)Traceback (most recent call last):  File "<stdin>", line 1, in <module>TypeError: pow() 3rd argument not allowed unless all arguments are integers

I'm not sure what fraction should do, maybe it should just continue not to support it? But accept the argument, and then print a better error message if modulo is not None?

Maybe it is worth adding tests for all the possible coercions too (ref:https://peps.python.org/pep-0208/)

CarlosEduR reacted with heart emoji
@wimglennwimglenn changed the titlefractions.Fraction __pow__ supports optional modulo argumentgh-119594: fractions.Fraction __pow__ to accept optional modulo argumentMay 27, 2024
Copy link
Member

@terryjreedyterryjreedy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Fraction.__pow__ should act same asfloat.__pow__. This new code does not.pow(3.0, 4, 5) andpow(3.5, 4, 5)` raise "TypeError: pow() 3rd argument not allowed unless all arguments are integers" Put following at top of code:

        if mod is not None:            raise TypeError("pow() 3rd argument not allowed unless all arguments are integers")

and correct the new test.

@bedevere-app
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

@mdickinson
Copy link
Member

Slight modification to Terry's suggestion: I think we can do:

ifmodisnotNone:returnNotImplemented

and then the general machinery should construct a reasonable error message for us. And intheory this gives other classes a chance to participate whereFraction declined to do so, but I'm not sure the delegation rules for ternary operations are particularly well-defined.

@wimglenn
Copy link
ContributorAuthor

wimglenn commentedMay 27, 2024
edited
Loading

I have made the requested changes; please review again.

Should__rpow__ also get the same treatment? Interestingly,pow(1, Fraction(1), 1) already gives the right error message but it looks as thoughFraction.__rpow__ does not get invoked at all in that case.I don't quite understand why.

edit: Found a sort-of answer in thedocs, where it says

Note that ternarypow() will not try calling__rpow__() (the coercion rules would become too complicated).

The ternary coercion rules inPEP 208 do seem complicated. So according to the status quo, seems a third argument for__rpow__ would be pointless (itwon't be used without changes in the C code).

@bedevere-app
Copy link

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.

@mdickinson
Copy link
Member

The ternary coercion rules inPEP 208 do seem complicated. So according to the status quo, seems a third argument for__rpow__ would be pointless (itwon't be used without changes in the C code).

Agreed, I think. I don't think it would be activelywrong to add the check to__rpow__, but it does look as though it wouldn't achieve anything (and so the status quo wins). Thanks for tracking down the relevant source - that was on my list of things to do.

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Should not__rpow__ be modified too?

@mdickinson
Copy link
Member

mdickinson commentedMay 28, 2024
edited
Loading

Should not__rpow__ be modified too?

The conclusion that@wimglenn came to in the comments above (and that I think I agree with) is that__rpow__ is never called for a ternarypow() call, so modifying__rpow__ would be pointless. I've just been staring at the relevant source code, and it's tangled enough that I can't tell forsure whether that's true or not.

serhiy-storchaka and wimglenn reacted with thumbs up emoji

@serhiy-storchaka
Copy link
Member

Ah, sorry, I did not read the discussion before looking at the code in this PR. I experimented and was not able to trigger calling__rpow__. It seems that it is only called inslot_nb_power_binary, which is only used when the third argument is None.

mdickinson reacted with thumbs up emoji

@mdickinson
Copy link
Member

I experimented and was not able to trigger calling__rpow__.

Yep, same here.

Copy link
Member

@mdickinsonmdickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM!

@terryjreedy Okay to merge?

@terryjreedyterryjreedy changed the titlegh-119594: fractions.Fraction __pow__ to accept optional modulo argumentgh-119594: Improve pow(a,b,modulo) error message if fractions.Fraction and modulo passedMay 29, 2024
@terryjreedyterryjreedy changed the titlegh-119594: Improve pow(a,b,modulo) error message if fractions.Fraction and modulo passedgh-119594: Improve pow(fraction.Fraction(), b, modulo) error messageMay 29, 2024
@terryjreedy
Copy link
Member

I am changing title and blurb to reflect user visible effect of change. Will then merge.

@terryjreedyterryjreedy merged commitfcca08e intopython:mainMay 29, 2024
@wimglennwimglenn deleted the pow3-fractions branchMay 29, 2024 18:05
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull requestJul 11, 2024
…ssage (python#119593)If one calls pow(fractions.Fraction, x, module) with modulo not None, the error message now says that the types are incompatible rather than saying pow only takes 2 arguments.  Implemented by having fractions.Fraction __pow__ accept optional modulo argument and return NotImplemented if not None.  pow() then raises with appropriate message.---------Co-authored-by: Mark Dickinson <dickinsm@gmail.com>
estyxx pushed a commit to estyxx/cpython that referenced this pull requestJul 17, 2024
…ssage (python#119593)If one calls pow(fractions.Fraction, x, module) with modulo not None, the error message now says that the types are incompatible rather than saying pow only takes 2 arguments.  Implemented by having fractions.Fraction __pow__ accept optional modulo argument and return NotImplemented if not None.  pow() then raises with appropriate message.---------Co-authored-by: Mark Dickinson <dickinsm@gmail.com>
scoder added a commit to scoder/quicktions that referenced this pull requestNov 28, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@serhiy-storchakaserhiy-storchakaserhiy-storchaka left review comments

@terryjreedyterryjreedyterryjreedy approved these changes

+1 more reviewer

@mdickinsonmdickinsonmdickinson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wimglenn@mdickinson@serhiy-storchaka@terryjreedy

[8]ページ先頭

©2009-2025 Movatter.jp