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

Add core::{f32,f64}::consts::TAU. #66769

Merged
merged 2 commits into from
Nov 28, 2019

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Nov 26, 2019

τ

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 26, 2019
src/libcore/num/f32.rs Outdated Show resolved Hide resolved
@kennytm
Copy link
Member

kennytm commented Nov 26, 2019

r? @dtolnay

@steveklabnik
Copy link
Member

I am not on the libs team, so my opinion doesn't really matter, but I don't think we should add this. Rust is not batteries included, and this just doesn't add enough value to justify itself.

@m-ou-se
Copy link
Member Author

m-ou-se commented Nov 26, 2019

Python has added this a few years ago, and it has become quite popular. (More popular than things like asinh for example.)

Considering the Rust standard library already comes with constants like FRAC_2_SQRT_PI, FRAC_PI_6 and LOG10_E, this seems like a rather small addition.

@RustyYato
Copy link
Contributor

RustyYato commented Nov 26, 2019

@steveklabnik given that we already have various fractions of pi (pi/2, pi/3, pi/4, pi/6, and pi/8), and adding a new const is super low cost, I don't think that there is much of a cost to adding TAU. Adding TAU allows thinking in terms of rotations instead of half rotations, which is significantly easier. So it does have really nice benefits.

We also have FRAC_2_SQRT_PI which is significantly more niche that TAU.

I realize that saying we have other constants is not an argument for adding another one, but the cost of adding a constant is almost nonexistent, so I don't see a reason to block it if there is some reason for adding it. Given that TAU makes thinking about rotations easier, and no one would pull a crate for a constant, I think this is fine.

@MikailBag
Copy link
Contributor

Related: https://blog.wolfram.com/2015/06/28/2-pi-or-not-2-pi/
Many usages of Pi should be replaced with Tau.
For example, if I want to get root of unity:

// with pi
use std::f64::{PI, sin, cos};
let omega = (cos(2*PI / n), sin(2*PI / n));
// with tau
use std::f64::{TAU, sin, cos};
let omega = (cos(TAU / n), sin(TAU / n));

You can see tau makes code simpler for understanding.

@m-ou-se , do you mind adding

  • link to tau manifesto (https://tauday.com/tau-manifesto),
  • definition of tau,
  • or some simple example that makes use of tau.
    to docs?
    Of course, Rust docs are not Math textbook, but tau is not well-known constant and maybe some explanation is worth addition.

@m-ou-se
Copy link
Member Author

m-ou-se commented Nov 26, 2019

@MikailBag The reason to add it to the standard library is that it's popular enough now that there are plenty of people who will want to use it. The point is not to use Rust's documentation to convice people to switch to tau (e.g. by linking the manifesto). I already put the definition (τ=2π) in the doc comments.

src/libcore/num/f32.rs Outdated Show resolved Hide resolved
@m-ou-se
Copy link
Member Author

m-ou-se commented Nov 26, 2019

Note also that two other new constants (LOG2_10, LOG10_2) are being stabilized right now because they are somewhat useful in specific use cases, and also exist in the D language: See #50540 and #50539

Considering tau has way more use cases, and has been in use in Python, Perl, Processing, Nim, Zig, and some others, I don't see why the idea of adding tau should lead to a big discussion every time.

@kennytm
Copy link
Member

kennytm commented Nov 27, 2019

@KrishnaSannasi We have FRAC_2_SQRT_PI because it was usually defined in C, not because it was commonly used. All stable numeric constants except π/3, π/6, π/8 (#6048) have corresponding C constants (once upon a time there was PI_2/two_pi() as well, but for some reason it was gone after the num reform).

Rust C
E M_E
FRAC_1_PI M_1_PI
FRAC_2_PI M_2_PI
FRAC_2_SQRT_PI M_2_SQRTPI
FRAC_1_SQRT_2 M_SQRT1_2
FRAC_PI_2 M_PI_2
FRAC_PI_3 -
FRAC_PI_4 M_PI_4
FRAC_PI_6 -
FRAC_PI_8 -
LN_2 M_LN2
LN_10 M_LN10
LOG2_E M_LOG2E
LOG10_E M_LOG10E
PI M_PI
SQRT_2 M_SQRT2
LOG10_2 -
LOG2_10 -
TAU -

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I am on board with adding a 2π constant. 2π appears commonly, and the justification that it's useful to have something that couples tightly (#66770 (comment)) is compelling to me. #66769 (comment) is compelling as well: "The point is not to use Rust's documentation to convince people to switch to tau (e.g. by linking the manifesto)." People are free to continue writing 2.0 * PI, just as they are free to continue using PI / 2.0 instead of our FRAC_PI_2 constant.

I added two checklist items to the tracking issue to follow up prior to stabilization:

  • Confirm that these numeric values are accurate. I did not do any validation other than eyeballing the numbers.
  • Decide whether we are comfortable using the name TAU or instead a longer but more widely accepted name like TWO_PI could be better.

@dtolnay
Copy link
Member

dtolnay commented Nov 27, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Nov 27, 2019

📌 Commit d220ed4 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 27, 2019
tmandry added a commit to tmandry/rust that referenced this pull request Nov 27, 2019
…nt, r=dtolnay

Add core::{f32,f64}::consts::TAU.

### **`τ`**
bors added a commit that referenced this pull request Nov 27, 2019
Rollup of 17 pull requests

Successful merges:

 - #64325 (Stabilize nested self receivers in 1.41.0)
 - #66222 (Use `eq_opaque_type_and_type` when type-checking closure signatures)
 - #66305 (Add by-value arrays to `improper_ctypes` lint)
 - #66399 (rustc_metadata: simplify the interactions between Lazy and Table.)
 - #66534 (Allow global references via ForeignItem and Item for the same symbol name during LLVM codegen)
 - #66700 (Fix pointing at arg for fulfillment errors in function calls)
 - #66704 (Intra doc enum variant field)
 - #66718 (Refactor `parse_enum_item` to use `parse_delim_comma_seq`)
 - #66722 (Handle non_exhaustive in borrow checking)
 - #66744 (Fix shrink_to panic documentation)
 - #66761 (Use LLVMDisposePassManager instead of raw delete in rustllvm)
 - #66769 (Add core::{f32,f64}::consts::TAU.)
 - #66774 (Clean up error codes)
 - #66777 (Put back tidy check on error codes)
 - #66797 (Fixes small typo in array docs r? @steveklabnik)
 - #66798 (Fix spelling typos)
 - #66800 (Combine similar tests for const match)

Failed merges:

r? @ghost
@scottmcm
Copy link
Member

Confirm that these numeric values are accurate. I did not do any validation other than eyeballing the numbers.

Because of floating point, I would expect 2.0 * PI to be perfectly accurate for this at all times, since it's just an exponent adjustment. Should it just be implemented that way to avoid the question?

@bors bors merged commit d220ed4 into rust-lang:master Nov 28, 2019
@solson
Copy link
Member

solson commented Nov 28, 2019

@scottmcm Would the same argument apply to FRAC_PI_2, FRAC_PI_4, and FRAC_PI_8? Perhaps somewhere someone has written down a best practices for numerical constants.

Edit: One can input the digits of pi and tau from this PR here in separate tabs and confirm they share the same significand, differing only by exponent. For a bit of fun, press "toggle details", and tick the exponent value up or down.

@m-ou-se m-ou-se deleted the tau-constant branch November 28, 2019 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.