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 pi_interval for Float32 and Float16 #337

Closed
wants to merge 1 commit into from

Conversation

mfherbst
Copy link
Contributor

Partially deals with #336

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 90.282% when pulling d86f7b1 on mfherbst:float32 into 1516c5e on JuliaIntervals:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 90.282% when pulling d86f7b1 on mfherbst:float32 into 1516c5e on JuliaIntervals:master.

@Kolaru
Copy link
Collaborator

Kolaru commented Dec 2, 2019

I haven't tested it for performance much yet, but in #271 I planned to achieve that through

const Interval{T}(::Irrational{:π}) where T = atomic(Interval{T}, π)
const Interval(::Irrational{:π}) = atomic(Interval{Float64}, π)

With that you are able to get pi_interval for any type using regular constructor, for example Interval{Float32}(π). Also this would allow to get rid of all pi_interval_* constant, but also requires more changes than your proposition.

If you think it fits your needs, I can look into having this as a separate PR.

@dpsanders
Copy link
Member

Do the const actually do anything there? The idea is definitely not to be recomputing this each time.

@Kolaru
Copy link
Collaborator

Kolaru commented Dec 2, 2019

I don't remember if the const is really needed (I'll test that), but the result should be computed only once when the function is compiled anyway, since the constructor do not depends on the value of the argument (and it works since the irrational pi is the only object that has type Irrational{:π}).

@mfherbst
Copy link
Contributor Author

mfherbst commented Dec 2, 2019

@Kolaru My main point for this PR was achieving a fix for #336, so if you propose a PR, which introduces a working set of trigonometric functions for Float32 intervals, then I'm happy, too 😄.

Actually using Irrational seems more like idiomatic Julia to me and less like the quick fix I proposed, so I'd surely appreciate if you could have a more polished go at it.

@dpsanders
Copy link
Member

@Kolaru That would be great if it works, but it's not clear to me that it will. (And no time to check right now.)

@Kolaru
Copy link
Collaborator

Kolaru commented Dec 3, 2019

julia> @btime Interval{Float16}(π)
  2.299 ns (0 allocations: 0 bytes)
[3.14062, 3.14258]

julia> @btime Interval{Float32}(π)
  0.001 ns (0 allocations: 0 bytes)
[3.14159, 3.1416]

julia> @btime Interval{Float64}(π)
  0.001 ns (0 allocations: 0 bytes)
[3.14159, 3.1416]

Somehow it looks like it works fine for Float32 and Float64, but not Float16, as confirmed by the following:

julia> @code_warntype Interval{Float16}(π)
Body::Interval{Float16}
1%1 = invoke IntervalArithmetic.atomic(Interval{Float16}::Type{Interval{Float16}}, IntervalArithmetic.π::Irrational{:π})::Interval{Float16}
└──      return %1

julia> @code_warntype Interval{Float32}(π)
Body::Interval{Float32}
1return [3.14159, 3.1416]

julia> @code_warntype Interval{Float64}(π)
Body::Interval{Float64}
1return [3.14159, 3.1416]

That's very mysterious to me, especially since the definition of both the constructor and of the atomic function are the same for Float32 and Float16.

EDIT:

The output is slighty different with Julia 1.3.0 (I was still on 1.1.0), but with the same conclusion, that is the compiler fail to infer the result is a constant for Float16.

julia> @code_warntype Interval{Float16}(π)
Variables
  #self#::Core.Compiler.Const(Interval{Float16}, false)
  #unused#::Core.Compiler.Const(π, false)

Body::Interval{Float16}
1%1 = Core.apply_type(IntervalArithmetic.Interval, $(Expr(:static_parameter, 1)))::Core.Compiler.Const(Interval{Float16}, false)
│   %2 = IntervalArithmetic.atomic(%1, IntervalArithmetic.π)::Interval{Float16}
└──      return %2

julia> @code_warntype Interval{Float32}(π)
Variables
  #self#::Core.Compiler.Const(Interval{Float32}, false)
  #unused#::Core.Compiler.Const(π, false)

Body::Interval{Float32}
1%1 = Core.apply_type(IntervalArithmetic.Interval, $(Expr(:static_parameter, 1)))::Core.Compiler.Const(Interval{Float32}, false)
│   %2 = IntervalArithmetic.atomic(%1, IntervalArithmetic.π)::Core.Compiler.Const([3.14159, 3.1416], false)
└──      return %2

julia> @code_warntype Interval{Float64}(π)
Variables
  #self#::Core.Compiler.Const(Interval{Float64}, false)
  #unused#::Core.Compiler.Const(π, false)

Body::Interval{Float64}
1%1 = Core.apply_type(IntervalArithmetic.Interval, $(Expr(:static_parameter, 1)))::Core.Compiler.Const(Interval{Float64}, false)
│   %2 = IntervalArithmetic.atomic(%1, IntervalArithmetic.π)::Core.Compiler.Const([3.14159, 3.1416], false)
└──      return %2

@Kolaru
Copy link
Collaborator

Kolaru commented Dec 5, 2019

The problem comes from the fact julia use specific mechanism to convert Irrational to Float64 or Float32, probably to exploit some hardware support. We can dodge the problem using generated functions (a feature I've always wanted to use anyway =D):

@generated function Interval{T}(x::Irrational) where T
    res = atomic(Interval{T}, x())
    return :($res)
end

This works as expected, precomputing the value the first time the function is called and subsequently reusing it. I'll thus be able to come with a PR to superseed the current one.

@lbenet
Copy link
Member

lbenet commented Dec 5, 2019

Can you clarify a bit more why it works with generated functions?

@Kolaru
Copy link
Collaborator

Kolaru commented Dec 6, 2019

Generated functions work by allowing you to compute the body of your functions from the types that are passed to it. In our case that allow us to to the following:

  1. Since each irrational has its own type, we can get the number by instantiating the type, e.g. we can get π by doing Irrational{:π}(). In the example it's what x() does, getting the value from the irrational type.
  2. We compute the atomic interval around the irrational for the type we need (the res variable).
  3. We define the body of the function to only consist of returning that value. It could be made a bit more explicit without omitting the return statement :(return $res).

So for each type, this define the function to simply return a constant.

@lbenet
Copy link
Member

lbenet commented Dec 6, 2019

Thanks a lot for the explanation!

@Kolaru
Copy link
Collaborator

Kolaru commented Dec 6, 2019

The test suite is currently testing for thing like

Interval{Float64}(π) == Interval{Float64}(float(π))

which fails with my proposed definition. Can I go forward and delete this kind if tests or is there a reason to have it that way ?

@dpsanders
Copy link
Member

Yes please go ahead and change whatever you need to. Thanks!

@Kolaru
Copy link
Collaborator

Kolaru commented Dec 6, 2019

Superseeded by #338. Thanks you very much for your input on the matter @mfherbst !

@Kolaru Kolaru closed this Dec 6, 2019
@mfherbst
Copy link
Contributor Author

mfherbst commented Dec 7, 2019

You're welcome. I'm happy to have started the discussion ;).

@mfherbst mfherbst deleted the float32 branch December 7, 2019 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants