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

Jump to bottom

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

Merged
merged 7 commits into from Jun 10, 2021
Merged

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

merged 7 commits into from Jun 10, 2021

Conversation

@ajthr
Copy link
Contributor

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

https://bugs.python.org/issue44357
@the-knights-who-say-ni
Copy link

@the-knights-who-say-ni 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 changes
Doc/library/math.rst Outdated Show resolved Hide resolved
@ajthr @mdickinson
Update Doc/library/math.rst
61aa722 
Co-authored-by: Mark Dickinson <dickinsm@gmail.com>
mdickinson
mdickinson reviewed Jun 9, 2021
View 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
Contributor Author

@ajthr ajthr Jun 9, 2021 •
edited

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 changes
Doc/whatsnew/3.10.rst Outdated
@@ -1093,6 +1093,12 @@ linecache
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
Contributor Author

@ajthr ajthr Jun 9, 2021 •
edited

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 Jun 9, 2021
@mdickinson
Copy link
Member

@mdickinson 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 Jun 9, 2021
@ajthr ajthr restored the dev branch Jun 9, 2021
@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 Jun 9, 2021
mdickinson
mdickinson approved these changes Jun 10, 2021
View changes
Copy link
Member

@mdickinson mdickinson left a comment

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
Contributor Author

@ajthr 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
12 checks passed
@ajthr ajthr deleted the dev branch Jun 10, 2021
JuniorJPDJ added a commit to JuniorJPDJ/cpython that referenced this issue Aug 12, 2021
@ajthr @blurb-it
@mdickinson @JuniorJPDJ
bpo-44357:Add math.cbrt() function: Cube Root (pythonGH-26622)
5d41c60 
* Add math.cbrt() function: Cube Root

Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Co-authored-by: Mark Dickinson <dickinsm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdickinson mdickinson

@serhiy-storchaka serhiy-storchaka

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
5 participants
@ajthr @the-knights-who-say-ni @mdickinson @serhiy-storchaka @bedevere-bot