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

Broaden array constructor to allow any fill value #41562

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rayegun
Copy link
Member

@rayegun rayegun commented Jul 12, 2021

It seems odd to me that Array{Union{Missing, T}}(missing, n...) and Array{Union{Nothing, T}}(nothing, n...) exist but the more general case Array{T}(x::T, n...) does not.

There are some potential issues I'm sure. For one it makes Array{T}(undef, n...) seem even more of an odd one out, rather than just the only strange one out of three options (the other two being nothing and missing). But I'm inclined to think this reduces the inconsistency since there are only two options, x::UndefInitializer and x::T after the change.

It seems odd to me that `Array{Union{Missing, T}}(missing, n...)` and `Array{Union{Nothing, T}}(nothing, n...)` exist but the more general case `Array{T}(x::T, n...)` does not. 

There are some potential issues I'm sure. Notably it makes `Array{T}(undef, n...)` seem even more of an odd one out, rather than just the only strange one out of three options (the other two being `nothing` and `missing`. But I'm inclined to think this reduces the inconsistency since there are only two options, `x::UndefInitializer` and `x::T`.
@fredrikekre
Copy link
Member

See #24595 and a bunch of linked issues/PRs in there.

@rayegun
Copy link
Member Author

rayegun commented Jul 12, 2021

I believe this syntax was actually proposed by you (4 years ago) deep in that issue.

This seems like a relatively limited subset of the proposal(s) there, while still allowing for all the extensions by restricting the type of x.

@fredrikekre
Copy link
Member

I believe this syntax was actually proposed by you (4 years ago) deep in that issue.

I am sure, but that's not really a convincing argument. In particular, this PR makes it impossible to ever support something like Vector{T}(it, 4) (iterate by default), Vector{T}(Rep(x), 4) (repeat x at every location), #33840, etc.

@JeffBezanson
Copy link
Sponsor Member

Another issue is that the 0-d case is ambiguous, e.g. Array{T}(other_array) already works to construct one array from another.

@JeffBezanson JeffBezanson added the domain:arrays [a, r, r, a, y, s] label Jul 12, 2021
@rayegun
Copy link
Member Author

rayegun commented Jul 12, 2021

I am sure, but that's not really a convincing argument.

Oh, no I was just noting that.

In particular, this PR makes it impossible to ever support something like Vector{T}(it, 4) (iterate by default), Vector{T}(Rep(x), 4) (repeat x at every location), #33840, etc.

Isn't the type restriction sufficient? Rep(x) and it shouldn't have the type T right?

Another issue is that the 0-d case is ambiguous, e.g. Array{T}(other_array) already works to construct one array from another.

I think this is the same right? The other array isn't type T.

@JeffBezanson
Copy link
Sponsor Member

I don't think it's a good idea for this to depend on the element type, but we could disambiguate it by requiring a dimension argument, e.g. Array(val, ()) for 0-d.

@rayegun
Copy link
Member Author

rayegun commented Jul 12, 2021

I don't think it's a good idea for this to depend on the element type

So it should (hypothetically) be broadened further to Array{T}(x, n...) instead of Array{T}(x::T, n...)? I don't get a method ambiguity for the latter and Array{T}(other_array), at least in my limited testing.

@JeffBezanson
Copy link
Sponsor Member

Ideally everything should work the same if the element type is widened; e.g. Array{Any}(x, ...) should do the same thing for all x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants