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

Inlining of small powers is incorrect #20768

Closed
dpsanders opened this issue Feb 23, 2017 · 12 comments
Closed

Inlining of small powers is incorrect #20768

dpsanders opened this issue Feb 23, 2017 · 12 comments

Comments

@dpsanders
Copy link
Contributor

dpsanders commented Feb 23, 2017

The new inlining of small powers is incorrect e.g. for interval arithmetic, where e.g. x^2 for x an Interval is not the same as x*x, and is defined using a completely separate code path.
E.g.:

using ValidatedNumerics

julia> x = -3..2   # interval from -3 to 2
[-3, 2]  

julia> x^2
[-6, 9]

julia> n = 2; x^n
[0, 9]

julia> x*x
[-6, 9]

cc @stevengj

@ajkeller34
Copy link
Contributor

You should be able to recover the behavior you want by importing ^ and defining ^{p}(x::ValidatedNumerics.Interval, ::Type{Val{p}}) = x^p.

@dpsanders
Copy link
Contributor Author

Yes, that works, thanks.
Still, it is counterintuitive that I should have to do that.

@JeffBezanson
Copy link
Sponsor Member

Haven't we always had a default definition of x^integer that computes x*x for x^2? What was happening before?

@ajkeller34
Copy link
Contributor

ajkeller34 commented Feb 23, 2017

I imagine this is probably happening because all of the specialized methods for ^ on Interval types had a concrete type for the second argument, such that the fallbacks were never called before. If there's no method ^(::Interval, ::Any) or ^{p}(::Interval, ::Type{Val{p}}) then the generic fallback will now be called. At least that's what seems to be happening at a glance.

edit: that's not quite right, there are some methods that have an abstract type for the second argument, but you get the idea.

@StefanKarpinski
Copy link
Sponsor Member

cc @stevengj

@stevengj
Copy link
Member

stevengj commented Feb 23, 2017

Probably this should be closed as a WONTFIX. x^2 before defaulted to x*x. It still defaults to x*x, so nothing has changed with regards to "correctness". The difference is that if you need to override this behavior you may need to override two methods of ^ instead of one.

This is the price that we pay for the opportunity for compile-time specialization and modified type computations for literal powers — we knew that going into this experiment. What we will find out in 0.6 is how many people get affected negatively by this; interval arithmetic is one such case. Hopefully there aren't too many others.

@stevengj
Copy link
Member

stevengj commented Feb 23, 2017

(I should also point out that x^2 = x*x is not technically "incorrect" for interval arithmetic. It just leads to overly pessimistic intervals, not wrong intervals, because of the dependency problem.)

@stevengj
Copy link
Member

stevengj commented Feb 23, 2017

An alternative would be to restrict the inlining of literal powers to hardware numeric types + complex numbers thereof, which gain the most from this.

@mbauman
Copy link
Sponsor Member

mbauman commented Feb 23, 2017

An interface where you have to know about and define two different methods in order to get a consistent specialization across two subtly different caller contexts seems to be troublesome in general. It's nice to provide the ability to override behaviors when you know the value of the exponent at compile-time, but could we structure the dispatch in a way that only base types avoid calling ^(x, ::Int)?

@andyferris
Copy link
Member

I thought the old behaviour was a codegen optimization for machine types?

@stevengj
Copy link
Member

#20782 changes inlining for literal exponents back to only hardware numeric types (and complex and rationals thereof), so it should close this issue.

@dpsanders
Copy link
Contributor Author

Confirmed fixed, many thanks!

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

No branches or pull requests

7 participants