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
Support zero
for arrays with heterogeneous eltypes
#41705
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1130,7 +1130,8 @@ function copymutable(a::AbstractArray) | |
end | ||
copymutable(itr) = collect(itr) | ||
|
||
zero(x::AbstractArray{T}) where {T} = fill!(similar(x, typeof(zero(T))), zero(T)) | ||
zero(x::AbstractArray{T}) where {T<:DataType} = fill!(similar(x, typeof(zero(T))), zero(T)) | ||
zero(x::AbstractArray{T}) where {T} = map(zero,x) #In case T is a TypeUnion b/c elements of x are heterogeneous types | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe check whether the resulting eltype Also, I wonder whether we could always use this code path, which would enable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Probably not always, we would at least have to special-case julia> zero([0,missing])
2-element Vector{Int64}:
0
0 (Although, maybe it would be better to return a For better performance, we could use the old method when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm trying to avoid that specific behavior here. It causes problems for Unitful There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, consistency of treatment would be nice, but I was also worried about performance, which is why I wrote it with two cases. I haven't done any profiling though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC, zero is usually unitless, but I think the move towards There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vtjnash What do you think about trying to distinguish those two cases by testing whether There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
How would that cause problems? You would still calculate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I misunderstood. There might be some benefit to that for consistency's sake. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I strongly recommend not using I'd say |
||
|
||
## iteration support for arrays by iterating over `eachindex` in the array ## | ||
# Allows fast iteration by default for both IndexLinear and IndexCartesian arrays | ||
|
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 you mean the case where
T isa DataType
here (which cannot be used in awhere
clause though, but would require anif
/else
in the same method). And you'd probably wantisconcretetype
instead.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.
Ahh, thank you! That's much better.