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

Support zero for arrays with heterogeneous eltypes #41705

Closed
wants to merge 2 commits into from

Conversation

lamorton
Copy link
Contributor

The motivation for this is to allow zero to work correctly for arrays of Unitful.Quantity that have different units. Cf. this issue at Unitful where it was suggested to make the fix in Base.

@Seelengrab
Copy link
Contributor

Ref. #33099

@@ -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))
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 you mean the case where T isa DataType here (which cannot be used in a where clause though, but would require an if/else in the same method). And you'd probably want isconcretetype instead.

Copy link
Contributor Author

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.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Maybe check whether the resulting eltype E is a strict subtype of T (E<:T && E!=T) and then convert the result to have eltype T?

Also, I wonder whether we could always use this code path, which would enable zero(x) even if zero(T) is not defined (e.g. T<:AbstractArray itself). Might have performance issues?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I wonder whether we could always use this code path

Probably not always, we would at least have to special-case T>:Missing to keep the current behavior:

julia> zero([0,missing])
2-element Vector{Int64}:
 0
 0

(Although, maybe it would be better to return a Vector{Union{Int,Missing}} here, but still filled with all zeros?)

For better performance, we could use the old method when T <: Number && isconcretetype(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.

Maybe check whether the resulting eltype E is a strict subtype of T (E<:T && E!=T) and then convert the result to have eltype T?

I'm trying to avoid that specific behavior here. It causes problems for Unitful Quantity because there's no concept of 'zero' for a generic quantity, only for specific quantities (eg: 0 meters is not 0 Kelvin is not 0 Celcius).

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 wonder whether we could always use this code path

For better performance, we could use the old method when T <: Number && isconcretetype(T).

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.

Copy link
Sponsor Member

@vtjnash vtjnash Jul 26, 2021

Choose a reason for hiding this comment

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

IIRC, zero is usually unitless, but I think the move towards g(a[i]) instead of g(eltype(a)) is generally a good one. But it does also assume that every element was previously defined and that you want to preserve the types of each element. So it is a choice between defining it to mean zero.(a) vs. fill!(a, 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 eltype is concrete? Is that 'too clever by half' or a reasonable way to infer what is intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe check whether the resulting eltype E is a strict subtype of T (E<:T && E!=T) and then convert the result to have eltype T?

I'm trying to avoid that specific behavior here. It causes problems for Unitful Quantity because there's no concept of 'zero' for a generic quantity, only for specific quantities (eg: 0 meters is not 0 Kelvin is not 0 Celcius).

How would that cause problems? You would still calculate y = map(zero, x) and check afterwards whether eltype(y) <: eltype(x), and if so convert y to AbstractArray{eltype(x)}. I don’t see how it could cause problems (whether it is desirable is another question, I think in some cases it might be).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

I strongly recommend not using map for this. The current map leaks the compiler internal and there's no guarantee exactly what eltype you are going to get when the input is empty.

I'd say map!(zero, similar(x), x) is a much better approach for this.

@sostock
Copy link
Contributor

sostock commented Jan 27, 2022

@lamorton Are you still working on this?

@lamorton
Copy link
Contributor Author

@sostock No, sorry.

@sostock
Copy link
Contributor

sostock commented Jan 27, 2022

Okay, I’ll work on a replacement for this.

@sostock sostock closed this Jan 27, 2022
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.

None yet

6 participants