-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8077587: BigInteger Roots #24898
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
base: master
Are you sure you want to change the base?
8077587: BigInteger Roots #24898
Conversation
…into nth-root-branch
@fabioromano1 Is this proof still relevant? |
@rgiulietti No, the proof of the recurrence used is the one by Zimmermann linked in the code. |
…into nth-root-branch
…into nth-root-branch
@fabioromano1 I guess this work will be the basis for a future work on Otherwise, there needs to be some evidence for useful use cases of general interest to justify adding n-th root to |
This reverts commit c8e1b81.
@rgiulietti Yes, it is. Some use cases could be primality testing, like AKS, where part of the algorithm detect composite numbers by checking whether an integer is an exact power or not. |
I doubt that the AKS primality test is of any practical relevance, although it's a great theoretical achievement. So if you plan to add a n-th root to |
Yes, indeed it was just an example, but the exact power detection can be used also as a test apart from the entire AKS algorithm. |
The Rampdown Phase One for JDK 25 is about 3 weeks from now. I think it's a bit too late for API additions to be approved in due time. And even if we could rush this work for 25, it makes little sense to have this in 25 and the future one in 26. IMO, they should be released together. So this and the followup PR for In other words, take your time ;-) |
@rgiulietti Anyway, I think the code has reached a stable version, so it is ready for review. |
@rgiulietti Regarding the tests for n-th root, the tests for |
@fabioromano1 Now that the jdk25 stabilization branch has been created, work on mainline (branch master) will no longer affect JDK25 (except for backports of bug fixes). |
@fabioromano1 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
/touch |
@fabioromano1 The pull request is being re-evaluated and the inactivity timeout has been reset. |
@fabioromano1 IIUC, the bulk of the code in the PR is aimed at finding a good starting estimate. Now, assume This has the following advantages:
I don't know if this has some negative impact on performance in practice, but IMO the resulting code is easier to review, understand, and maintain. Maybe worth giving it a try. |
@rgiulietti It should be noted that the convergence of Newton's method is quadratic, meaning the number of correct bits doubles with each iteration. This means that, to keep the number of iterations low, the larger the input, the greater the need for as many initial correct bits as possible. Moreover, the initial estimate gives at most 53 bits, so delegating its computation to |
This PR implements nth root computation for BigIntegers using Newton method.
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24898/head:pull/24898
$ git checkout pull/24898
Update a local copy of the PR:
$ git checkout pull/24898
$ git pull https://git.openjdk.org/jdk.git pull/24898/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24898
View PR using the GUI difftool:
$ git pr show -t 24898
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24898.diff
Using Webrev
Link to Webrev Comment