Skip to content
This repository has been archived by the owner on Mar 12, 2021. It is now read-only.

Simplify array constructors. #172

Merged
merged 2 commits into from
Oct 31, 2018
Merged

Simplify array constructors. #172

merged 2 commits into from
Oct 31, 2018

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Oct 29, 2018

CuArrays/GPUArrays currently define lots of constructors (and methods of similar and convert) that have diverged from Base and really should not exist. This PR tries to get rid of them, to improve maintainability and code portability.

As part of that work, I propose to move essential functionality such as constructors to the concrete packages (ie. CuArrays). Defining these methods in a generic fashion is cumbersome, introduces loads of ambiguities, and results in applicable but nonsensical method invocations. For example:

julia> convert(GPUArray, cu([1]))
ERROR: MethodError: no method matching GPUArray{Float32,N} where N(::UndefInitializer, ::Tuple{Int64})
Stacktrace:
 [1] similar(::Type{GPUArray{Float32,N} where N}, ::Tuple{Int64}) at ./abstractarray.jl:618
 [2] convert(::Type{GPUArray}, ::CuArray{Float32,1}) at /home/tbesard/Julia/GPUArrays/src/construction.jl:97
 [3] top-level scope at none:0

This because of a generic Base.convert(::Type{<: GPUArray{T1}}, ...) that is only defined to be invoked on concrete subtypes. Base doesn't attempt to share such functionality between Array and AbstractArray.

Obviously massively breaking, but seems to pass tests except for some CUSOLVER failure.

@codecov-io
Copy link

codecov-io commented Oct 30, 2018

Codecov Report

Merging #172 into master will decrease coverage by 0.08%.
The diff coverage is 77.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
- Coverage   43.56%   43.47%   -0.09%     
==========================================
  Files          34       34              
  Lines        2128     2123       -5     
==========================================
- Hits          927      923       -4     
+ Misses       1201     1200       -1
Impacted Files Coverage Δ
src/subarray.jl 64.7% <ø> (ø) ⬆️
src/gpuarray_interface.jl 28.57% <ø> (-6.22%) ⬇️
src/fft/genericfft.jl 56.25% <0%> (ø) ⬆️
src/broadcast.jl 33.33% <0%> (-11.12%) ⬇️
src/dnn/nnlib.jl 58.82% <100%> (ø) ⬆️
src/blas/wrap.jl 52.16% <100%> (ø) ⬆️
src/rand/highlevel.jl 82.75% <100%> (ø) ⬆️
src/solver/highlevel.jl 53.57% <33.33%> (ø) ⬆️
src/fft/CUFFT.jl 52.82% <66.66%> (ø) ⬆️
src/array.jl 49.29% <73.91%> (-1.38%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa73ebe...0fef350. Read the comment docs.

@maleadt maleadt changed the title WIP: Simplify array constructors. Simplify array constructors. Oct 30, 2018
@maleadt maleadt merged commit 599942e into master Oct 31, 2018
@maleadt maleadt deleted the tb/simplify branch October 31, 2018 12:17
@andreasnoack
Copy link
Member

Hm. Master is currently failing a ton of tests for me and I suspect that this PR is involved but it looks like CI went through just fine.

@maleadt
Copy link
Member Author

maleadt commented Nov 1, 2018

GPUArrays from master? The current runtests.jl matches branches on CI.

@andreasnoack
Copy link
Member

No that was the problem. I'd only checked out CuArrays master.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants