Skip to content

bpo-44357:Add math.cbrt() function: Cube Root #26622

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.

Sign up for GitHub

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

Jump to bottom
Merged
mdickinson merged 7 commits into from
Jun 10, 2021
Merged

bpo-44357:Add math.cbrt() function: Cube Root #26622

mdickinson merged 7 commits into from
Jun 10, 2021

Conversation

@ajthr
Copy link

@ajthr ajthr commented Jun 9, 2021 •
edited by bedevere-bot
Loading

@the-knights-who-say-ni
Copy link

the-knights-who-say-ni commented Jun 9, 2021

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@AjithRamachandran

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

mdickinson
mdickinson reviewed Jun 9, 2021
View reviewed changes
@mdickinson
Update Doc/library/math.rst
61aa722 
Co-authored-by: Mark Dickinson <[email protected]>
mdickinson
mdickinson reviewed Jun 9, 2021
View reviewed changes
Lib/test/test_math.py
self.ftest('cbrt(27)', math.cbrt(27), 3)
self.ftest('cbrt(-1)', math.cbrt(-1), -1)
self.ftest('cbrt(-27)', math.cbrt(-27), -3)
self.assertEqual(math.cbrt(INF), INF)
Copy link
Member

@mdickinson mdickinson Jun 9, 2021

Choose a reason for hiding this comment

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

Please could you also add some tests for non-integral cases? It would be good to have a check that cbrt(-0.0) returns -0.0 (with the correct sign), too.

Copy link
Author

@ajthr ajthr Jun 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

👍

Copy link
Member

@serhiy-storchaka serhiy-storchaka Jun 9, 2021

Choose a reason for hiding this comment

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

But the test for sqrt() does not contain tests for non-integral cases.

Copy link
Member

@mdickinson mdickinson Jun 9, 2021

Choose a reason for hiding this comment

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

@serhiy-storchaka That's true, but I'm not sure why you're pointing it out in this PR discussion. Can you clarify? I'd be very happy to review a PR that fixed that defect for math.sqrt, but I think it's off-topic for this PR.

mdickinson
mdickinson reviewed Jun 9, 2021
View reviewed changes
Doc/whatsnew/3.10.rst Outdated
When a module does not define ``__loader__``, fall back to ``__spec__.loader``.
(Contributed by Brett Cannon in :issue:`42133`.)
math
Copy link
Member

@mdickinson mdickinson Jun 9, 2021

Choose a reason for hiding this comment

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

This is in the wrong "whatsnew" document; this would be a change for Python 3.11, not Python 3.10.

Copy link
Author

@ajthr ajthr Jun 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

👍

@mdickinson mdickinson self-requested a review June 9, 2021 09:58
@mdickinson
Copy link
Member

mdickinson commented Jun 9, 2021

Thank you for the PR! I'll do a proper review later today (UTC+1).

@ajthr ajthr closed this Jun 9, 2021
@ajthr ajthr deleted the dev branch June 9, 2021 15:35
@ajthr ajthr restored the dev branch June 9, 2021 15:35
@ajthr ajthr reopened this Jun 9, 2021
@ajthr ajthr changed the title bpo-44357:Add math.cbrt() function: Cube Root bpo-44357:Add math.cbrt() function: Cube Root Jun 9, 2021
@ajthr ajthr requested a review from serhiy-storchaka June 9, 2021 20:43
mdickinson
mdickinson approved these changes Jun 10, 2021
View reviewed changes
Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM. I took the liberty of pushing a commit which tweaks the line spacing a bit, for consistency. Will merge as soon as the checks complete.

Thank you for the contribution!

@ajthr
Copy link
Author

ajthr commented Jun 10, 2021

LGTM. I took the liberty of pushing a commit which tweaks the line spacing a bit, for consistency. Will merge as soon as the checks complete.

Thank you for the contribution!

ty :)

@mdickinson mdickinson merged commit ac867f1 into python:main Jun 10, 2021
@ajthr ajthr deleted the dev branch June 10, 2021 20:56
@jopemachine jopemachine mentioned this pull request Sep 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

1 more reviewer

@mdickinson mdickinson mdickinson 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.

5 participants

@ajthr @the-knights-who-say-ni @mdickinson @serhiy-storchaka @bedevere-bot