Skip to content

Commit

Permalink
Remove a type restriction in Base.QuadGK.Segment (#19626) (#19627)
Browse files Browse the repository at this point in the history
* Remove a type restriction in Base.QuadGK.Segment (#19626)

* Test compatibility of quadgk with unitful numbers / physical quantities.

* Pare down tests.

* More test refinements.
  • Loading branch information
ajkeller34 authored and stevengj committed Dec 23, 2016
1 parent 44d7677 commit c428a5f
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 1 deletion.
2 changes: 1 addition & 1 deletion base/quadgk.jl
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ immutable Segment
a::Number
b::Number
I
E::Real
E
end
isless(i::Segment, j::Segment) = isless(i.E, j.E)

Expand Down
26 changes: 26 additions & 0 deletions test/math.jl
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,32 @@ end
@test quadgk(cos, 0,0.7,1, norm=abs)[1] sin(1)
end

module Test19626
using Base.Test

# Define a mock physical quantity type
immutable MockQuantity <: Number
val::Float64
end

# Following definitions needed for quadgk to work with MockQuantity
import Base: +, -, *, abs, isnan, isinf, isless
+(a::MockQuantity, b::MockQuantity) = MockQuantity(a.val+b.val)
-(a::MockQuantity, b::MockQuantity) = MockQuantity(a.val-b.val)
*(a::MockQuantity, b::Number) = MockQuantity(a.val*b)
abs(a::MockQuantity) = MockQuantity(abs(a.val))
isnan(a::MockQuantity) = isnan(a.val)
isinf(a::MockQuantity) = isinf(a.val)
isless(a::MockQuantity, b::MockQuantity) = isless(a.val, b.val)

# isapprox only needed for test purposes
Base.isapprox(a::MockQuantity, b::MockQuantity) = isapprox(a.val, b.val)

# Test physical quantity-valued functions
@test quadgk(x->MockQuantity(x), 0.0, 1.0, abstol=MockQuantity(0.0))[1]
MockQuantity(0.5)
end

@testset "subnormal flags" begin
# Ensure subnormal flags functions don't segfault
@test any(set_zero_subnormals(true) .== [false,true])
Expand Down

11 comments on commit c428a5f

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@KristofferC
Copy link
Member

Choose a reason for hiding this comment

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

Does this look a bit weird? So many at exactly 0.5 ratio.

@jrevels
Copy link
Member

Choose a reason for hiding this comment

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

Maybe an artifact of a change in code layout?

Let's see what happens if we run it again. Note that this will execute two runs, one after the other, and compare their results, as opposed to executing a single run then comparing against the previous day's results.

@nanosoldier runbenchmarks(ALL, vs = "@f27c6f3ae50b45e0e6ff2305dd5031d07c8665a7")

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@jrevels
Copy link
Member

Choose a reason for hiding this comment

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

Well, at least it's consistent.

@KristofferC
Copy link
Member

Choose a reason for hiding this comment

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

Hehe OK then. I was just surprised with the number of quite different benchmarks that exactly got 2x faster.

@stevengj
Copy link
Member

Choose a reason for hiding this comment

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

Still doesn't make sense. This patch does not touch any code that is used by BaseBenchmarks.

@KristofferC
Copy link
Member

@KristofferC KristofferC commented on c428a5f Dec 24, 2016

Choose a reason for hiding this comment

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

This is a daily benchmark so perhaps #17057?

@KristofferC
Copy link
Member

@KristofferC KristofferC commented on c428a5f Dec 25, 2016

Choose a reason for hiding this comment

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

Cant be.. Improvements are on very basic operations as well.

@KristofferC
Copy link
Member

Choose a reason for hiding this comment

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

Like how could isinteger on BigFloat get a factor of 2 faster?

Please sign in to comment.