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
Contributor

m-ou-se commented Nov 26, 2019

τ

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 26, 2019

r? @kennytm

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

@m-ou-se m-ou-se force-pushed the fusion-engineering-forks:tau-constant branch from 489fbd5 to 83763ff Nov 26, 2019
src/libcore/num/f32.rs Outdated Show resolved Hide resolved
@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Nov 26, 2019

@rust-highfive rust-highfive assigned dtolnay and unassigned kennytm Nov 26, 2019
@m-ou-se m-ou-se mentioned this pull request Nov 26, 2019
1 of 2 tasks complete
@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Nov 26, 2019

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

This comment has been minimized.

Copy link
Contributor 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.

@KrishnaSannasi

This comment has been minimized.

Copy link
Contributor

KrishnaSannasi 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

This comment has been minimized.

Copy link
Contributor

MikailBag commented Nov 26, 2019

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

This comment has been minimized.

Copy link
Contributor 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 m-ou-se force-pushed the fusion-engineering-forks:tau-constant branch from 3eb11bb to d220ed4 Nov 26, 2019
@m-ou-se

This comment has been minimized.

Copy link
Contributor 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

This comment has been minimized.

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 left a comment

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

This comment has been minimized.

Copy link
Member

dtolnay commented Nov 27, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 27, 2019

📌 Commit d220ed4 has been approved by dtolnay

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

This comment has been minimized.

Copy link
Member

scottmcm commented Nov 28, 2019

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
4 checks passed
4 checks passed
pr #20191126.44 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@solson

This comment has been minimized.

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 fusion-engineering-forks:tau-constant branch Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.