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

JDK-8283124: Add constant for tau to Math and StrictMath #7813

Closed
wants to merge 3 commits into from

Conversation

jddarcy
Copy link
Member

@jddarcy jddarcy commented Mar 14, 2022

Add a constant for tau, 2pi, to Math and StrictMath. Since 2pi is a very common value in mathematical formulas, it is helpful to give it a distinct constant.

Please also review the CSR https://bugs.openjdk.java.net/browse/JDK-8283136


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed
  • Change requires a CSR request to be approved

Issues

  • JDK-8283124: Add constant for tau to Math and StrictMath
  • JDK-8283136: Add constant for tau to Math and StrictMath (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7813/head:pull/7813
$ git checkout pull/7813

Update a local copy of the PR:
$ git checkout pull/7813
$ git pull https://git.openjdk.java.net/jdk pull/7813/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7813

View PR using the GUI difftool:
$ git pr show -t 7813

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7813.diff

@jddarcy
Copy link
Member Author

jddarcy commented Mar 14, 2022

/csr needed

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 14, 2022

👋 Welcome back darcy! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added csr Pull request needs approved CSR before integration rfr Pull request is ready for review labels Mar 14, 2022
@openjdk
Copy link

openjdk bot commented Mar 14, 2022

@jddarcy an approved CSR request is already required for this pull request.

@openjdk
Copy link

openjdk bot commented Mar 14, 2022

@jddarcy The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Mar 14, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 14, 2022

Webrevs

bplb
bplb approved these changes Mar 14, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 14, 2022

Mailing list message from Raffaello Giulietti on core-libs-dev:

Hello,

I find it a bit disturbing that PI is specified with 21 digits whereas
TAU has 16.
I think that specifying PI as
public static final double PI = 3.141592653589793;
doesn't harm anybody and makes it visually more consistent with TAU-

Greetings
Raffaello

On 3/14/22 22:13, Brian Burkhalter wrote:

@mlbridge
Copy link

mlbridge bot commented Mar 14, 2022

Mailing list message from Hans Boehm on core-libs-dev:

Couldn't the apiNote just say TAU == 2 * PI instead? I think the fact that
this is actually a guaranteed floating point equality aids clarity.

On Mon, Mar 14, 2022 at 2:33 PM Raffaello Giulietti <
raffaello.giulietti at gmail.com> wrote:

Hello,

I find it a bit disturbing that PI is specified with 21 digits whereas
TAU has 16.
I think that specifying PI as
public static final double PI = 3.141592653589793;
doesn't harm anybody and makes it visually more consistent with TAU-

Greetings
Raffaello

On 3/14/22 22:13, Brian Burkhalter wrote:

On Mon, 14 Mar 2022 20:52:39 GMT, Joe Darcy <darcy at openjdk.org> wrote:

Add a constant for tau, 2*pi, to Math and StrictMath. Since 2*pi is a
very common value in mathematical formulas, it is helpful to give it a
distinct constant.

Please also review the CSR
https://bugs.openjdk.java.net/browse/JDK-8283136

@mlbridge
Copy link

mlbridge bot commented Mar 14, 2022

Mailing list message from Raffaello Giulietti on core-libs-dev:

Right, and PI with 16 digits (or 17).

On 3/14/22 22:51, Hans Boehm wrote:

Couldn't the apiNote just say TAU == 2 * PI instead? I think the fact
that this is actually a guaranteed floating point equality aids clarity.

On Mon, Mar 14, 2022 at 2:33 PM Raffaello Giulietti
<raffaello.giulietti at gmail.com <mailto:raffaello.giulietti at gmail.com>>
wrote:

Hello\,

I find it a bit disturbing that PI is specified with 21 digits whereas
TAU has 16\.
I think that specifying PI as
 \? \? \?public static final double PI \= 3\.141592653589793\;
doesn\'t harm anybody and makes it visually more consistent with TAU\-


Greetings
Raffaello



On 3\/14\/22 22\:13\, Brian Burkhalter wrote\:
 > On Mon\, 14 Mar 2022 20\:52\:39 GMT\, Joe Darcy \<darcy at openjdk\.org
\<mailto\:darcy at openjdk\.org>> wrote\:
 >
 >> Add a constant for tau\, 2\*pi\, to Math and StrictMath\. Since 2\*pi
is a very common value in mathematical formulas\, it is helpful to
give it a distinct constant\.
 >>
 >> Please also review the CSR
https\:\/\/bugs\.openjdk\.java\.net\/browse\/JDK\-8283136
\<https\:\/\/bugs\.openjdk\.java\.net\/browse\/JDK\-8283136>

@jddarcy
Copy link
Member Author

jddarcy commented Mar 14, 2022

Mailing list message from Hans Boehm on core-libs-dev:

Couldn't the apiNote just say TAU == 2 * PI instead? I think the fact that this is actually a guaranteed floating point equality aids clarity.

On Mon, Mar 14, 2022 at 2:33 PM Raffaello Giulietti < raffaello.giulietti at gmail.com> wrote:

Hello,
I find it a bit disturbing that PI is specified with 21 digits whereas
TAU has 16.
I think that specifying PI as
public static final double PI = 3.141592653589793;
doesn't harm anybody and makes it visually more consistent with TAU-
Greetings
Raffaello
On 3/14/22 22:13, Brian Burkhalter wrote:

On Mon, 14 Mar 2022 20:52:39 GMT, Joe Darcy wrote:

Add a constant for tau, 2pi, to Math and StrictMath. Since 2pi is a
very common value in mathematical formulas, it is helpful to give it a
distinct constant.
Please also review the CSR
https://bugs.openjdk.java.net/browse/JDK-8283136

Yes; after further thought, I agree having tau = 2.0*pi is preferable.

Just to go through the logic, 2.0 is exactly representable in binary floating-point and an in-range multiple by two is just an exponent adjustment.

Floating-point exponent transitions occur at 2.0 = 0x1 * 2^1, 4.0 = 0x1 * 2^2, and 8.0 = 0x1 * 2^3. The value of pi is between 2.0 and 4.0 and has an exponent of 1 while the value of tau ~= 6.28 is between 4.0 and 8.0 and has an exponent of 2.

So whatever the closest floating-point value to exact pi is, 2.0 * Math.pi will be the closest floating-point value to tau.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Mar 14, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 14, 2022

Mailing list message from Joseph D. Darcy on core-libs-dev:

Hi Raffaello,

With changing TAU to be set to 2.0 * PI, I'll file a follow-up bug to
use the least-precision decimal values that will get rounded to PI and
E, respectively, in Math and StrictMath. (Per the general base
conversion properties for the double format, there will be between 15
and 17 decimal digits rather than 21.)

Thanks,

-Joe

On 3/14/2022 2:32 PM, Raffaello Giulietti wrote:

Hello,

I find it a bit disturbing that PI is specified with 21 digits whereas
TAU has 16.
I think that specifying PI as
??? public static final double PI = 3.141592653589793;
doesn't harm anybody and makes it visually more consistent with TAU-

Greetings
Raffaello

On 3/14/22 22:13, Brian Burkhalter wrote:

@openjdk
Copy link

openjdk bot commented Mar 14, 2022

@jddarcy This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8283124: Add constant for tau to Math and StrictMath

Reviewed-by: bpb, iris

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 14, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 15, 2022

Mailing list message from Kevin Bourrillion on core-libs-dev:

On Mon, Mar 14, 2022 at 4:16 PM Joe Darcy <darcy at openjdk.java.net> wrote:

Yes; after further thought, I agree having tau = 2.0*pi is preferable.

Kinda reads like it's taking a stand on which of the two is the *real*
fundamental mathematical constant and which one is a hack. Therefore it
should really be pi that's set to tau/2.0 :-)

Can PI have a @see link to this so that people know about it?

(I'll just mention that {from,to}Radians would also benefit from some
advertisement, as a great many PI usages are dividing it by or into 180,
but do what thou wilt with that FYI.)

--
Kevin Bourrillion | Java Librarian | Google, Inc. | kevinb at google.com

@mlbridge
Copy link

mlbridge bot commented Mar 15, 2022

Mailing list message from Kevin Bourrillion on core-libs-dev:

I did a subjective eval of 30 random usages of Math.PI in our codebase. I
would repeat this in more detail if it were seen as useful.

These usages really wanted --

* Math.toRadians (6)
* Math.TAU (10) [usually, they are explicitly doubling it]
* Math.PI (1) [some high-pass filter formula uses a bare pi]
* subject to taste (13)

By the last one, I mean that the more a person has read, understood, and
agreed with the Tau Manifesto the more they would prefer to use TAU in that
situation. For example, for code that's talking about rotating an image a
"quarter-turn", or the angle measure in a rectangle, TAU / 4 is very
natural. But what about clamping a longitude expressed in radians -- is it
cleaner to think that the absolute value is less than (a) one halfturn (PI)
or (b) one-half of a turn (TAU/2)? Who can say.

I suppose that introducing a new cause for subjective arguments between
teammates is no a small thing. But still, the 10-to-1 part sells it for me.
Again, I could flesh this out if necessary.

(On a side note, the real-world value for "they should just use
to/fromRadians" will be *much* higher than this. We actually have static
analyzers that have been trying to nag people into using those methods for
years, so these 6/30 are just the ones we didn't catch.)

On Mon, Mar 14, 2022 at 6:23 PM Kevin Bourrillion <kevinb at google.com> wrote:

On Mon, Mar 14, 2022 at 4:16 PM Joe Darcy <darcy at openjdk.java.net> wrote:

Yes; after further thought, I agree having tau = 2.0*pi is preferable.

Kinda reads like it's taking a stand on which of the two is the *real*
fundamental mathematical constant and which one is a hack. Therefore it
should really be pi that's set to tau/2.0 :-)

Can PI have a @see link to this so that people know about it?

(I'll just mention that {from,to}Radians would also benefit from some
advertisement, as a great many PI usages are dividing it by or into 180,
but do what thou wilt with that FYI.)

--
Kevin Bourrillion | Java Librarian | Google, Inc. | kevinb at google.com

--
Kevin Bourrillion | Java Librarian | Google, Inc. | kevinb at google.com

@jddarcy
Copy link
Member Author

jddarcy commented Mar 15, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Mar 15, 2022

Going to push as commit 05a83e0.

@openjdk openjdk bot added the integrated Pull request has been integrated label Mar 15, 2022
@openjdk openjdk bot closed this Mar 15, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 15, 2022
@openjdk
Copy link

openjdk bot commented Mar 15, 2022

@jddarcy Pushed as commit 05a83e0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
3 participants