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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expunging @ngenerate and @nsplat #9098

Merged
merged 9 commits into from
Feb 7, 2015
Merged

Expunging @ngenerate and @nsplat #9098

merged 9 commits into from
Feb 7, 2015

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Nov 21, 2014

This one's for you, @JeffBezanson. Happy early thesis-defense.

Now that we have stagedfunctions/@generated functions, there is no need for @ngenerate. (@nsplat still might have a purpose, but without @ngenerate I think we might as well ditch both at once. You can fake de-splatting with stagedfunctions.) This is a WIP at getting rid of them. Currently if I try to convert those last few BitArray methods, I get failures: building hangs at primes.jl, the first file after multidimensional.jl. Interestingly, they all involve @ncall, leading me to suspect a type-inference problem. #8504 is a likely suspect.

This is broken up in a series of small steps because it was basically murder to debug this---these functions are core infrastructure, and with multiple breakages it's hard to figure out what's happening. Given that we don't fully trust stagedfunctions yet (see #8501 (comment)), I also thought it would be good to ensure that git bisect would be as informative as possible. But if people object, I can (reluctantly) squash. Just don't ask me to bisect any problems later 馃槃.

See also JuliaLang/Compat.jl#19, which hopefully will provide backward-compatibility. I know this wasn't an exported interface, but I still felt guilty about just yanking it. I'll also write up some docs about how to convert functions.

@timholy
Copy link
Sponsor Member Author

timholy commented Nov 21, 2014

What's this "trailing whitespace found in file" and how do I easily get rid of it?

@jiahao
Copy link
Member

jiahao commented Nov 21, 2014

pwned by #8914

I don't know what your favourite editor is but I'm sure googling 'remove trailing whitespace ' should do the trick.

@timholy
Copy link
Sponsor Member Author

timholy commented Nov 21, 2014

Thanks. Just did a git rebase --whitespace=fix HEAD~5, hopefully that will do the trick.

@ivarne
Copy link
Sponsor Member

ivarne commented Nov 21, 2014

That command should be a suggestion for the whitespace fail.

@timholy timholy changed the title WIP: expunging @ngenerate and @nsplat Expunging @ngenerate and @nsplat Feb 3, 2015
@timholy
Copy link
Sponsor Member Author

timholy commented Feb 3, 2015

It seems the real obstacle to further progress on this PR was recursive staged functions (#8853), which was fixed by the merger of #9921. I've gone ahead and finished the removal of @ngenerate and @nsplat from base. I've also removed the WIP from the title. Finally, I've verified that JuliaLang/Compat.jl#19, once merged, suffices to get Images.jl (which uses @ngenerate heavily, as well as @nsplat) to pass its tests.

@Jutho
Copy link
Contributor

Jutho commented Feb 4, 2015

Looks great. As soon as this gets merged, I will revisit the problem of implementing a more general/more efficient version of permutedims!.

@JeffBezanson
Copy link
Sponsor Member

Nice net deletion!

@timholy
Copy link
Sponsor Member Author

timholy commented Feb 4, 2015

Thanks.

I'm traveling over the next few days, so I'll hold off on merging this until I can deal with any problems that arise.

timholy added a commit that referenced this pull request Feb 7, 2015
Expunging @ngenerate and @nsplat
@timholy timholy merged commit 4fd7fef into master Feb 7, 2015
@timholy timholy deleted the teh/ngenerate branch February 7, 2015 13:05
@IainNZ
Copy link
Member

IainNZ commented Feb 9, 2015

This broke an impressive amount of packages (sorry for delayed reaction), but I think it comes down to StatsBase.jl - JuliaStats/StatsBase.jl#107

@timholy
Copy link
Sponsor Member Author

timholy commented Feb 9, 2015

I'm not remotely surprised. Looks like @simonster has fixed the problem in StatsBase, and as long as a new version has been tagged things should clear out tomorrow.

Still may be some individual ones that need fixing, like JuliaMath/Interpolations.jl#21 (comment)

@IainNZ
Copy link
Member

IainNZ commented Feb 9, 2015

Certainly one of the easier fixes! Its nice to see things breaking again :D

@simonster
Copy link
Member

This also breaks DataArrays even though it's using Compat. I haven't dug into why yet. I see some ambiguity warnings that weren't there before:

Warning: New definition 
    getindex(DataArrays.DataArray{T,N},Union(AbstractArray{T,1},Real)...) at /home/simon/.julia/Compat/src/ngenerate.jl:53
is ambiguous with: 
    getindex(DataArrays.AbstractDataArray{T,N},Real) at /home/simon/.julia/DataArrays/src/indexing.jl:104.
To fix, define 
    getindex(DataArrays.DataArray{T,N},Real)
before the new definition.
Warning: New definition 
    getindex(DataArrays.DataArray{T,N},Union(AbstractArray{T,1},Real)...) at /home/simon/.julia/Compat/src/ngenerate.jl:53
is ambiguous with: 
    getindex(AbstractArray{T,N},Real) at abstractarray.jl:434.
To fix, define 
    getindex(DataArrays.DataArray{T,N},Real)
before the new definition.
Warning: New definition 
    getindex(DataArrays.PooledDataArray{T,R<:Integer,N},Union(AbstractArray{T,1},Real)...) at /home/simon/.julia/Compat/src/ngenerate.jl:53
is ambiguous with: 
    getindex(DataArrays.AbstractDataArray{T,N},Real) at /home/simon/.julia/DataArrays/src/indexing.jl:104.
To fix, define 
    getindex(DataArrays.PooledDataArray{T,R<:Integer,N},Real)
before the new definition.
Warning: New definition 
    getindex(DataArrays.PooledDataArray{T,R<:Integer,N},Union(AbstractArray{T,1},Real)...) at /home/simon/.julia/Compat/src/ngenerate.jl:53
is ambiguous with: 
    getindex(AbstractArray{T,N},Real) at abstractarray.jl:434.
To fix, define 
    getindex(DataArrays.PooledDataArray{T,R<:Integer,N},Real)
before the new definition.

(edit: this is just because the stagedfunction has a more general method signature and is easy to fix) and then this throws an error (with a somewhat bogus-looking backtrace):

julia> DataArray(randn(100))[1:100]
ERROR: UndefVarError: I_1 not defined
 in anonymous at /home/simon/.julia/DataArrays/src/indexing.jl:30
 in _getindex at /home/simon/.julia/DataArrays/src/indexing.jl:141
 in getindex at /home/simon/.julia/DataArrays/src/indexing.jl:152

simonster added a commit to JuliaStats/DataArrays.jl that referenced this pull request Feb 9, 2015
simonster added a commit to JuliaStats/DataArrays.jl that referenced this pull request Feb 9, 2015
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.

None yet

7 participants