-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Tests and bugfix for sharedarrays #11252
Conversation
@@ -249,7 +249,7 @@ function rand!{T}(S::SharedArray{T}) | |||
end | |||
|
|||
function randn!(S::SharedArray) | |||
f = S->map!(x->randn, S.loc_subarr_1d) | |||
f = S->map!(x->randn(), S.loc_subarr_1d) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this can't just be:
f = S->randn!(S.loc_subarr_1d)
(similarly for rand!
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's because that variable is a SubArray
, for which rand!
and randn!
were not defined in 0.3.
It could be changed now I suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked randn!
with @simonster's suggestion on 0.4 and it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be only defined for AbstractArray{Float64}
though, while it would be nice to keep the SharedArray
code generic (for example, in the previous version it was generically and uniformly failing... ehm).
See also #9836.
On the other hand, rand!
should be fine AFAICT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The discussion in #9836 suggests that converting the result of randn()
to other floating point types is not quite right. If we're not defining randn!(::Array{Float32})
because of this, then I don't think randn!(::SharedArray{Float32})
should work either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Computing in 64 bit and converting to 32 should be mostly fine, from what I gather. But I'm ok with the proposed change anyway, it makes sense to rely on any implementation AbstractArray
provides.
Wow, this has been there for quite a while. Thanks! |
Tests and bugfix for sharedarrays
backported in 4d66b75, with one small change where the exception type was different on release-0.3 |
randn(S)
whereS
is aSharedArray
would error out with aconvert
error. Added a test for this,rand(S)
,reshape
,similar
, andsdata
.