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

Deprecate ^(x, p::Integer) #23332

Closed
wants to merge 2 commits into from

Conversation

iamed2
Copy link
Contributor

@iamed2 iamed2 commented Aug 18, 2017

This caused method ambiguities when defining ^(::MyType, ::Real) which I think is reasonably common when overloading ^.
I ran tests I thought were related and got no failures; we'll see what CI says about the full suite.
power_by_squaring makes a bunch of Number-related assumptions so I doubt anyone was using this in the wild.

@iamed2
Copy link
Contributor Author

iamed2 commented Aug 18, 2017

Fun fact: this method predates the announcement of Julia

@ararslan ararslan added deprecation This change introduces or involves a deprecation maths Mathematical functions labels Aug 18, 2017
@fredrikekre
Copy link
Member

I don't think this is the right fix, to make integer powers work for your own type you only need to overload *, but now that will throw a depwarn.

julia> struct Foo x end

julia> Base.:*(x::Foo, y::Foo) = Foo(x.x*y.x)

julia> x = Foo(2)
Foo(2)

julia> x^3
WARNING: x ^ p::Integer is deprecated, use power_by_squaring(x, p) instead.
Stacktrace:
 [1] depwarn at ./deprecated.jl:68 [inlined]
 [2] ^(::Foo, ::Int64) at ./deprecated.jl:56
 [3] literal_pow(::Base.#^, ::Foo, ::Val{3}) at ./intfuncs.jl:214
 [4] eval(::Module, ::Expr) at ./repl/REPL.jl:3
 [5] eval_user_input(::Any, ::Base.REPL.REPLBackend) at ./repl/REPL.jl:69
 [6] macro expansion at ./repl/REPL.jl:100 [inlined]
 [7] (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:96
while loading no file, in expression starting on line 0
Foo(8)

@iamed2
Copy link
Contributor Author

iamed2 commented Aug 18, 2017

Is there a case where you would do that and your type is not a Number subtype? Number types still work.

@iamed2
Copy link
Contributor Author

iamed2 commented Aug 18, 2017

Your type has to implement one as well for that function to work.

@fredrikekre
Copy link
Member

Is there a case where you would do that and your type is not a Number subtype?

I don't know, but I am sure someone does it :) In general, it is nice that you don't have to overload everything to get things to work; if you define * you get integer powers for free!

@iamed2
Copy link
Contributor Author

iamed2 commented Aug 18, 2017

Alternate proposal:

^(x, p) = power_by_squaring(x, p)

Then you could freely override ^ for more generic p.

@tkelman
Copy link
Contributor

tkelman commented Aug 18, 2017

power_by_squaring isn't valid for non-integer powers though

@fredrikekre
Copy link
Member

If you define

^(::MyType, x::Real) = ...
^(::MyType, x::Integer) = ...

everything works, right? Probably these methods are different anyways since you can do some faster stuff in the Integer case.

@iamed2
Copy link
Contributor Author

iamed2 commented Aug 18, 2017

@fredrikekre No, those methods are not different in this case.

@tkelman You would get a method error on power_by_squaring (which still requires an integer argument) instead of the method error you'd get on ^. I don't think that's much different than the current situation.

@fredrikekre
Copy link
Member

^(x::MyType, p::Real) = mypow(x, p)
^(x::MyType, p::Integer) = mypow(x, p)
mypow(x::MyType, p::Real) = ...

I just worry that since this is such a general method that is deprecated, it will essentially look like ^ is deprecated for some cases, and the above workaround is quite simple :)

@iamed2
Copy link
Contributor Author

iamed2 commented Aug 19, 2017

It definitely isn't that simple in our case: https://github.com/invenia/Nabla.jl/blob/master/src/sensitivities/scalar.jl

@tkelman
Copy link
Contributor

tkelman commented Aug 19, 2017

I don't think that's much different than the current situation.

It would lead at least some people to think they should be extending power_by_squaring for their types, which doesn't necessarily make a lot of sense.

@JeffBezanson JeffBezanson added the triage This should be discussed on a triage call label Jan 31, 2018
@JeffBezanson
Copy link
Member

Triage accepts.

@JeffBezanson JeffBezanson added this to the 1.0 milestone Feb 1, 2018
@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Feb 1, 2018
@JeffBezanson JeffBezanson added the needs news A NEWS entry is required for this change label Feb 2, 2018
@omus omus deleted the remove-untyped-caretpow branch February 6, 2018 23:15
@stevengj
Copy link
Member

stevengj commented Sep 5, 2018

@alanedelman just complained to me about this because he likes to do classroom demos where he defines Base.:*(f::Function, g::Function) = f ∘ g and then calls sin^2, which no longer works because there is no ^ fallback anymore. 😿

@StefanKarpinski
Copy link
Member

Well, there's always the option of defining ^(f::Function, n::Integer) = power_by_squaring(f, n) directly but then there's the question of whether that's the correct implementation or not. For functions, it seems like generating a function that iteratively calls the function n types would be a better implementation, which suggests that removing this fallback was still the right call.

@KristofferC KristofferC removed the needs news A NEWS entry is required for this change label Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants