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

use the compact f(x::AbstractVector{<:Number}) syntax #33

Open
mkborregaard opened this issue Aug 23, 2017 · 8 comments
Open

use the compact f(x::AbstractVector{<:Number}) syntax #33

mkborregaard opened this issue Aug 23, 2017 · 8 comments

Comments

@mkborregaard
Copy link

I have

ignorenan_minimum{F<:AbstractFloat}(x::AbstractArray{F}) = NaNMath.minimum(x)

the bot suggests

ignorenan_minimum(x::AbstractArray{F}) where {F<:AbstractFloat} = NaNMath.minimum(x)

but I'd prefer

ignorenan_minimum(x::AbstractArray{<:AbstractFloat}) = NaNMath.minimum(x)

here: JuliaPlots/Plots.jl#1036 (comment)

@Keno
Copy link
Member

Keno commented Aug 24, 2017

Those do actually mean different things to the system. I guess @JeffBezanson should judge whether or not this rewrite would be ok.

@mkborregaard
Copy link
Author

Yeah I guess it requires F to never appear in the function body. If there are other important differences that would also be really useful to know :-)

@JeffBezanson
Copy link
Member

Fortunately (on master) those should dispatch exactly the same (or else we'd now consider it a bug). However, @mkborregaard is right that this transformation is only safe if F is not a free variable of the body, which is pretty difficult to compute. But it should certainly be safe if F doesn't appear at all in the body.

This variation would also be good to support:

f(x::T) where T<:Real = ...

to

f(x::Real) = ...

(also, of course, where T doesn't appear in the body).

@Keno
Copy link
Member

Keno commented Aug 25, 2017

Ok, can do. Just making sure we don't rely on this in specialization heuristics, etc.

@JeffBezanson
Copy link
Member

Yes, I think it still affects specialization, but the cases where that's needed should be very rare. We can deal with those cases individually as they arise. I might even suggest that if you think you need such specialization but don't have the benchmarks to prove it, then you don't really :)

@Keno
Copy link
Member

Keno commented Aug 25, 2017

Should we have some sort of macro that people can use to indicate that they really do want the specialization behavior (similar to what we did with what used to be ::ANY).

@StefanKarpinski
Copy link
Member

At that point, wouldn't it be better to let them just directly ask for the specialization, instead of having it be coupled with the way you write the method signature and telling the bot not to undo it?

@mkborregaard
Copy link
Author

Just to hear if I'm understanding this correctly - my impression is that at some point this will be implemented, when that happens the femtocleaner PR will be automatically updated, and I should just hold on merging that until then?

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

4 participants