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

Fix Base.mapreduce to match the new signature in 0.7/1.0 #145

Merged
merged 5 commits into from
Aug 27, 2018

Conversation

ssz66666
Copy link
Contributor

In Julia 0.7/1.0, the signature of Base.mapreduce and its alternative function Base.mapreducedim which allows a dims argument, have changed to use keyword arguments, i.e.
from
mapreduce(f, op[, v0], itr)
mapreducedim(f, op, itr, region[, v0])
to
mapreduce(f, op, itr; dims=:, [init])

It seems that the bot wasn't able to figure out that this is deprecated syntax. The current head is still passing tests since no test case tries to pass a customised initial value to it.

…d in Julia 0.7/1.0, namely moving `dims` and `v0` into kwargs
@ssz66666 ssz66666 changed the title change Base.mapreduce to match the new signature in 0.7/1.0 Fix Base.mapreduce to match the new signature in 0.7/1.0 Aug 22, 2018
@vchuravy
Copy link
Member

Thanks! It would be greatly appreciated if you can add testcases for this as well!

@ssz66666
Copy link
Contributor Author

ssz66666 commented Aug 25, 2018

Apologies for the messy commit that combines different changes I made. I'll try to make a new branch for each fix next time.
Changes I made (based on master):

  1. try to fix Base.show, which includes two variants:

Base.show(io::IO, ::MIME"text/plain", X::GPUArray), which is used by e.g. display to show a human-readable array on REPL.
This calls print_array.

Base.show(io::IO, X::GPUArray), which is used by e.g. print to show a string that is close to its literal value string, like you write [1] to create an Array{Int64,1} with 1 as its sole element.
This calls show_vector for vectors, _show_empty for empty non-vector arrays, and _show_nonempty for non-empty non-vector arrays.

  1. Add test cases for:
    showing: I included use of both variants of Base.show as well as the case of showing a 2 by 2 matrix.
    mapreduce: as suggested by @vchuravy , included a test case to check calls with keyword args.

  2. Add GPUArrays.allowscalar(false) in test(AT::Type{<:GPUArray})#src/testsuite.jl. This does not seem to be enforced by default and thus allows some scalar index errors to sneak through unit test/CI.


for (atype, op) in
[(:(GPUArray), :(Array)),
(:(LinearAlgebra.Adjoint{<:Any,<:GPUArray}), :(x->LinearAlgebra.adjoint(Array(x.parent)))),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use Base.parent instead of accessing the field directly

src/testsuite.jl Outdated
@@ -49,6 +49,7 @@ end
Runs the entire GPUArrays test suite on array type `AT`
"""
function test(AT::Type{<:GPUArray})
GPUArrays.allowscalar(false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my reasoning here was that implementations can decide themselves if they want to assert scalar indexing, eg. https://github.com/JuliaGPU/CuArrays.jl/blob/master/test/runtests.jl#L27
but then it probably needs to be added to the JLArray tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe moving scalar indexing assertion into runtest.jl of GPUArrays package for JLArray tests, like

@testset "JLArray" begin
    local _prev = GPUArrays._allowscalar[]
    GPUArrays.allowscalar(false)
    GPUArrays.test(JLArray)
    GPUArrays._allowscalar[] = _prev
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like that, yeah. But we can leave that for a different PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(something like @allowscalar, eg. @disallowscalar would be cleaner then, not having to work with unexported values in GPUArrays)

@ssz66666
Copy link
Contributor Author

Fixed wrapper field access, will submit a separate PR for the test suite behaviour change.

@maleadt
Copy link
Member

maleadt commented Aug 27, 2018

Thanks! Too bad we can't run CI on this, guess we'll have to keep a close eye on master after merging PRs like this.

@maleadt maleadt merged commit e10a9b9 into JuliaGPU:master Aug 27, 2018
@maleadt
Copy link
Member

maleadt commented Aug 27, 2018

OK, so this apparently broke CuArrays (as per CI), due to passing a CPU array to the GPU. Did you test with CuArrays.jl? Once JuliaLang/METADATA.jl#17380 is merged CI will report better errors.

EDIT: ah yeah, this is just mapreduce being implemented without kwargs in GPUArrays.

@ssz66666
Copy link
Contributor Author

ssz66666 commented Aug 27, 2018

Hmm, don't know why the added unit test broke CuArrays. CUDAnative fails to compile the GPU kernel for some reason. Right now it seems that it is the occurrence of anonymous function _addone that is causing the problem. manually running the code in Main outside the testsuite seemed to work fine...

EDIT4: @maleadt I've submitted another PR #151 to attempt to workaround the problem. It seems to me the problem here is CUDAnative cannot handle non-bits types as kernel function parameter, with the exception of a few types, such as CuArray (obviously). A function(closure) capturing a local scope non-bit type variable is itself of non-bits type, thus is not a valid parameter for a GPU kernel.
EDIT3: fix typo
EDIT2: See the code below. Totally confused.

julia> begin
           local a = 1
           local b = Int32
           local c = one(b)
           local d = Ref(1)
           local f(x) = x
           local g(x) = x + a
           local h(x) = x + one(b)
           const j(x) = x + one(b)
           local k(x) = x + c
           local l(x) = x + d[]
           println(isbitstype(typeof(f)))
           println(isbitstype(typeof(g)))
           println(isbitstype(typeof(h)))
           println(isbitstype(typeof(j)))
           println(isbitstype(typeof(k)))
           println(isbitstype(typeof(l)))
       end
true
true
false
true
true
false

EDIT1: not an anonymous function

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.

3 participants