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

Zeros #19635

Merged
merged 9 commits into from
Dec 22, 2016
Merged

Zeros #19635

merged 9 commits into from
Dec 22, 2016

Conversation

jw3126
Copy link
Contributor

@jw3126 jw3126 commented Dec 17, 2016

Fix #19265 and add methods to zeros / ones with same signature as similar.

zs = zeros(SparseMatrixCSC([1 2; 3 4]), Complex{Float64}, (2,3))
test_zeros(zs, SparseMatrixCSC{Complex{Float64}}, (2, 3))

@test_throws Exception zeros(Float64, [1.]) #19265
Copy link
Contributor

Choose a reason for hiding this comment

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

which exception type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tests that the situation in #19265 throws an error. It is a method error, so one could test for method error explicitly. I thought about this, but I think the precise exception type is an implementation detail and does not need to be part of the spec?

Copy link
Contributor

Choose a reason for hiding this comment

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

should test specifically, otherwise a typo in the implementation could pass this test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

@tkelman
Copy link
Contributor

tkelman commented Dec 17, 2016

add a test for the situation from #19265 to make sure it doesn't alias now?

@jw3126
Copy link
Contributor Author

jw3126 commented Dec 18, 2016

I think this PR is ready.

@ViralBShah
Copy link
Member

I think this could use a slightly better commit message. Would you mind squashing and adding a couple of lines?

* Fix JuliaLang#19265.
* Add methods to zeros, ones with analgous signature to similar.
@jw3126
Copy link
Contributor Author

jw3126 commented Dec 20, 2016

Done.

@stevengj
Copy link
Member

Needs a NEWS item.

@@ -97,6 +97,8 @@ Library improvements
That is, not every member of the input iterable will be visited if a `true` (in the case of `any`) or
`false` (in the case of `all`) value is found, and `mapreduce` will visit all members of the iterable.

* Additional methods for `ones` and `zeros` functions. These have the same signature as the `similar` function.
Copy link
Member

Choose a reason for hiding this comment

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

* Additional methods for `ones` and `zeros` functions to support the same signature as the `similar` function ([#19635]).

function ($fname)(a::AbstractArray, T::Type=eltype(a), dims::Tuple=size(a))
fill!(similar(a,T,dims), $felt(T))
end
($fname)(T::Type, dims::Tuple) = ($fname)(Array{T}(dims...), T, dims)
Copy link
Member

Choose a reason for hiding this comment

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

This isn't right: it allocates an Array{T}(dims...) and passes it to $fname(...), but then that function calls similar, which allocates an second array. So you end up allocating one more array than you need.

I think you just want

($fname)(T::Type, dims::Tuple) = fill!(Array{T}(dims...), $felt(T))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

($fname)(T::Type, dims::Tuple) = ($fname)(Array{T}(dims...), T, dims)
($fname)(dims::Tuple) = ($fname)(Float64, dims)

($fname)(a::AbstractArray,T::Type,dims::DimOrInd...) = ($fname)(a,T,dims)
Copy link
Member

Choose a reason for hiding this comment

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

DimOrInd (an alias for Union{AbstractUnitRange{T},Integer}) doesn't seem right here. Shouldn't it just be Integer...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stole this from similar, which also uses DimOrInd for some methods. I think DimOrInd is for exotic things like OffsetArrays?

@@ -564,9 +564,10 @@ julia> ones(Complex128, 2, 3)
ones(t,dims)

"""
ones(A)
ones(A::AbstractArray, T=eltype(A)::Type, dims=size(A)::DimOrInd)
Copy link
Member

Choose a reason for hiding this comment

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

this type signature isn't right for dims

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right!

($fname)(T::Type, dims::Tuple) = fill!(Array{T}(dims...), $felt(T))
($fname)(dims::Tuple) = ($fname)(Float64, dims)

($fname)(a::AbstractArray,T::Type,dims::DimOrInd...) = ($fname)(a,T,dims)
Copy link
Member

Choose a reason for hiding this comment

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

dims::Integer...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stole this from similar, which also uses DimOrInd for some methods. I think DimOrInd is for exotic things like OffsetArrays?

Copy link
Member

Choose a reason for hiding this comment

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

okay... abstractarray.jl calls to_shape(dims) to get a tuple, though, and similar also supports dims::NeedsShaping

Copy link
Member

@stevengj stevengj Dec 21, 2016

Choose a reason for hiding this comment

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

Maybe it would be better to just define:

$fname(a::AbstractArray, T::Type, dims::Tuple) = fill!(similar(a,T,dims), $felt(T))
$fname(a::AbstractArray, T::Type, dims...) = fill!(similar(a,T,dims...), $felt(T))
$fname(a::AbstractArray, T::Type=eltype(a)) = fill!(similar(a,T), $felt(T))

so that it can take any dims argument supported by similar and has the same behavior for exotic array types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mhh okay, I was not aware of NeedsShaping. I will think about a clean way to support this, if I don't find one, I will switch back to Integer as you suggested in the beginning.

Copy link
Member

Choose a reason for hiding this comment

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

The NeedsShaping cases will be handled by the dims::Tuple method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

# exotic indexing
oarr = zeros(randn(3), UInt16, 1:3, -1:0)
@test indices(oarr) == (1:3, -1:0)
test_zeros(oarr.parent, Matrix{UInt16}, (3, 2))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stevengj Here is a test that shows why ::DimOrInd instead of ::Integers was choosen.

$fname(a::AbstractArray, T::Type, dims...) = fill!(similar(a,T,dims...), $felt(T))
$fname(a::AbstractArray, T::Type=eltype(a)) = fill!(similar(a,T), $felt(T))

$fname(T::Type, dims::Tuple) = fill!(Array{T}(Dims(dims)...), $felt(T))
Copy link
Member

Choose a reason for hiding this comment

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

No need for splatting here; you can just do fill!(Array{T}(Dims(dims)), $felt(T)) I think?

@@ -564,9 +564,10 @@ julia> ones(Complex128, 2, 3)
ones(t,dims)

"""
ones(A)
ones(A::AbstractArray, T=eltype(A)::Type, dims=size(A)::Tuple)
Copy link
Member

Choose a reason for hiding this comment

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

Need to document the dims... case too. Is ones(T, dims...) documented somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are at least some doctests where dims... is used. I can be more explicit about it.

@stevengj
Copy link
Member

LGTM.

@ViralBShah ViralBShah merged commit 66ab171 into JuliaLang:master Dec 22, 2016
Create an array of all ones of specified type. The type defaults to `Float64` if not specified.
Create an array of all ones with the same layout as `A`, element type `T` and size `dims`.
The `A` argument can be skipped, which behaves like `Array{Float64,0}()` was passed.
For convenience `dims` may also be passed in variadic form.
Copy link
Contributor

Choose a reason for hiding this comment

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

"variadic form" is not very clear, we don't use that terminology anywhere else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you call it instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd spell out an example

Copy link
Member

Choose a reason for hiding this comment

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

Maybe:

For convenience, `dims` may be passed as individual integer arguments, as in `zeros(3,4,5)`, rather than as a tuple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are examples where dims... is used already e.g. zeros(Int8, 2, 3). But I can open a new PR with stevengj's suggestion if you like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I think if we don't have a terminology for dims..., we should introduce one. It occurs quite frequently and feels cumbersome not having a word for it.

Copy link
Member

Choose a reason for hiding this comment

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

I think "variadic form" is fine, but it should be spelled out with an example as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

without any explanation that's terribly jargony and not very clear

Copy link
Member

Choose a reason for hiding this comment

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

Then please make a PR that improves it.

MichaelHatherly added a commit to MichaelHatherly/julia that referenced this pull request Dec 23, 2016
JuliaLang#19635 merged two docstrings for
each of `zeros` and `ones` in a single docstring each. This changes the
signatures in the stdlib file to reflect that change.
@MichaelHatherly MichaelHatherly mentioned this pull request Dec 23, 2016
MichaelHatherly added a commit to MichaelHatherly/julia that referenced this pull request Dec 24, 2016
JuliaLang#19635 merged two docstrings for
each of `zeros` and `ones` in a single docstring each. This changes the
signatures in the stdlib file to reflect that change.
@timholy
Copy link
Member

timholy commented Jan 5, 2017

Worth noting, the syntaxes zeros(A, dims...) conflicts with something else we might like, zeros(-2:2, 0:5) to create an all-zeros array with indices (-2:2, 0:5). That first range argument is an AbstractArray so it will get captured by the zeros(A, ...) method.

@timholy
Copy link
Member

timholy commented Jan 5, 2017

Oh, looks like you test for this, but the true meaning is actually ambiguous unless the user supplies an intervening T. If ndims(A) > 1 you can also figure out what was intended, but the 1d case is awkward. For example, is zeros(1:3, 1:5) supposed to create an all-zeros vector "similar to a UnitRange{Int}", or is it supposed to create a 3x5 Matrix{Float64}?

@jw3126
Copy link
Contributor Author

jw3126 commented Jan 5, 2017

Good point! I think at the moment zeros(1:3, 1:5) will be an error. Of course one could use dims instead of dims... to be unambiguous in these cases. But I agree it is awkward.

@timholy
Copy link
Member

timholy commented Jan 5, 2017

As long as the ambiguous case (not in the julia-method-sense, but in the "what do I mean?" sense) throws an error, we're probably OK.

@timholy
Copy link
Member

timholy commented Feb 24, 2017

Seems like we need a depwarn.

Julia 0.5:

julia> ones(Float64, 1:5)
5-element Array{Float64,1}:
 1.0
 1.0
 1.0
 1.0
 1.0

master:

julia> ones(Float64, 1:5)
ERROR: MethodError: Cannot `convert` an object of type UnitRange{Int64} to an object of type Int64
This may have arisen from a call to the constructor Int64(...),
since type constructors fall back to convert methods.
Stacktrace:
 [1] ones(::Type{T} where T, ::UnitRange{Int64}) at ./array.jl:252

@timholy
Copy link
Member

timholy commented Feb 24, 2017

Note that this depwarn should be viewed as a bugfix, and thus allowed even though we're in feature freeze.

@timholy
Copy link
Member

timholy commented Mar 27, 2017

Bump @jw3126 (see the depwarn above).

@jw3126
Copy link
Contributor Author

jw3126 commented Mar 27, 2017

I can take care of the depwarn next weekend.

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.

6 participants