Implement tau==2pi (in type-stable manner)

[waldyrious]

Not sure why I was focused on overriding * all this time. Writing the tau-2pi-equality.md document helped me get a better grasp of the problem, and I realized that adding custom methods for == not only makes more sense, but is also simpler -- and type-stable :)

[giordano]

I don't think I like this. This should be implemented in julia core for all irrational numbers if there is consensus on such change, I don't see why tau should be special in this regard :)

[waldyrious]

I'd have no problems making a proposal to Base, but what exactly would you suggest the change to be? Here we're allowing two Irrationals to have an exact proportionality relationship with each other (because their mathematical definition pretty much hinges on that relationship), while remaining unequal to everything else, as defined in base. Unless I'm mistaken, none of the other Irrationals in Base do have this sort of relationship with each other.

[codecov]

Codecov Report

Merging #28 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #28   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines          61     67    +6     
=====================================
+ Hits           61     67    +6

Impacted Files	Coverage Δ
src/Tau.jl	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be5d27f...ddeb839. Read the comment docs.

[waldyrious]

Note: while I was at it, I took the opportunity to reorganize the test script and start using the @testset macro. I hadn't realized how many combinations there were in the sintau/costau tests in total -- almost 10,000! I think we might be ok dropping that number a bit by using larger steps and smaller ranges for the approximate output tests, which are the biggest culprits.

[giordano]

I would split the reorganization of tests from this PR, that change is unrelated and unquestionable

[waldyrious]

Good point. I've rebuilt the testset reorganization changes independently, in #29. I'll rebase this branch over it so that it contains only the equality changes.

[giordano]

I'd have no problems making a proposal to Base, but what exactly would you suggest the change to be?

A possibility could be

==(x::Irrational, y::T) where {T<:AbstractFloat} = T(x) == y
==(x::T, y::Irrational) where {T<:AbstractFloat} = x == T(y)

In this way:

julia> pi == 3.1415927f0
true

julia> 3.141592653589793 == pi
true

julia> 3.141592653589794 == pi
false

julia> pi == big"3.141592653589793238462643383279502884197169399375105820974944592307816406286198"
true

But speaking for myself I find the argument "an Irrational can't be equal to any decimal number with finite representation" pretty compelling.

[waldyrious]

I see, so every irrational would be equal to (various representations of) itself. I will collect some information from previous discussions, and submit a PR later.

In the meantime, I've rebased this PR to cleanup the changes after the merging of #29. I suppose this change (making 2pi equal to tau) can be handled separately to making every irrational equal to itself, since with this change the original inequality definition remains in place for any other combination.

The exception for tau==2pi is IMO justifiable, given that this is precisely how these two constants are defined -- them not being equal is more likely to cause confusion than the fact that an irrational number is considered equal to what is technically a finite representation. The latter only trips the user if they're analyzing the underlying representation of their code from the point of view of how the computer handles it, rather than whatever conceptual algorithm they're implementing. Even then, that surprise is not likely to cause practical issues, since this change has no performance implications (that I'm aware of).

[giordano]

The exception for tau==2pi is IMO justifiable, given that this is precisely how these two constants are defined

So you're saying that pi is more fundamental than tau? I thought that in this package was the other way around ;-)

[waldyrious]

You know what I mean :P

(edit: just for the sake of completeness: tau = circumference/radius and pi = circumference/diameter and diameter = 2*radius, therefore tau = 2*pi ⟺ pi = tau/2)

[mschauer]

As you write in the document, in julia 2π is rational because of rounding. So 2π!=tau, in the same way as

julia> π^2/π != π
true

I would not do an ad hoc fix to this by redefining equality.

[waldyrious]

Thanks for your input, @mschauer :)

in julia 2π is rational because of rounding

Yes, and this is a technical implementation artifact, not a deliberate choice. The current inequality is actually based on mixing two extremes of the abstraction spectrum: one based on the low-level technical limitations of floating point number representation, and another based on the high-level mathematical properties of irrational numbers. I'm not entirely convinced that combining these two concepts, from very distinct (almost opposite) abstraction levels, as premises for reaching the conclusion of inequality (and going out of our way to manually define such inequality, which otherwise the promotion mechanism would handle as one would intuitively expect) is a sound choice.

I would not do an ad hoc fix to this by redefining equality.

That was exactly what was done in JuliaLang/julia#9198, though: defining an override to the equality method (and a hardcoded one at that!), so that it returned the the result it should. according to the author of that change. The solution presented here has exactly the same kind of rationale (ensuring the result of tau==2pi is what it should be, per the mathematical definition), and exactly the same kind of implementation. I don't see why it's objectionable on those grounds in particular.

One objection I'm willing to concede to is that perhaps a more perfect way to do this could be found -- but until an implementation plan is defined, and considering that this change maintains type stability and has no performance impact (nor it affects the strict inequality for all other Irrational comparisons), I'm inclined to go ahead and merge in honor of Tau day tomorrow. We can always replace this implementation with something better down the road, and I'm very much open to that.

[mschauer]

After your change either

julia> x <= y < z <= x
true

with x, y, z = τ, 2π, (2*big(π)+ big(2π))/2
(or

julia> tau == 2pi
true

julia> tau <= 2pi
false

if you do not recompile <=)

[giordano]

Lines 20, 21, and 29 are acts of type-piracy of Base's types.

[giordano]

Yes, and this is a technical implementation artifact, not a deliberate choice.

This isn't actually true, it is a deliberate choice not making arithmetic operations for Irrationals, see JuliaLang/julia#21535 (comment)

Taking the risk of repeating myself, I'm against such change, it should be either done in Base or not done at all here. tau is a cool and useful constant, but not more special than others in Base, it shouldn't be treated differently.

[waldyrious]

Good point @mschauer, I had forgotten about that. This is not exclusive to this PR, though -- comparison operators are broken for all Irrationals (and I've been meaning to submit a fix for that, but haven't had the time yet). Until the inequality operators are fixed for Irrationals in general, I don't think it's unreasonable to ignore them in Tau.jl for now.

[waldyrious]

@giordano I understand. I was hoping we could get this merged for Tau day, but I guess it'll have to wait for next year. I'll propose changes to Base before insisting further on this.

[mschauer]

@waldyrious Thanks, I think that is a good idea.

[giordano]

Anyway a new version of Tau.jl can still be tagged today, there have been quite some changes and fixes since last version ;-)

[waldyrious]

Good point, @giordano. I'll go ahead and tag a new version. Hopefully we can get it merged to METADATA.jl still today :)

[waldyrious]

@giordano please check https://github.com/JuliaMath/Tau.jl/releases/tag/v0.1.1 and JuliaLang/METADATA.jl#10008 :)

[MasonProtter]

I actually kinda like the proposal to have

==(x::Irrational, y::T) where {T<:AbstractFloat} = T(x) == y
==(x::T, y::Irrational) where {T<:AbstractFloat} = x == T(y)

but if that never ends up in Base, then I'd argue that you just want to warn people that

julia> τ == 2π
false

is the wrong comparison to make and instead they should do the comparison

julia> 1τ == 2π
true

[devmotion]

I'll close this PR as it is quite old, has conflicts with the main branch, and uses outdated syntax.

Regarding the proposed change, IMO Tau should be consistent with base, and the policy there hasn't changed (I think this policy is reasonable). Apart from the checks

julia> 1τ == 2π
true

or

julia> Float64(τ) == 2π
true

based on Float64, an alternative based on irrationals would be

julia> using IrrationalConstants

julia> τ == twoπ
true

Both constants are even identical objects 🙂