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

make I consistently provide identity behavior in type #24396

Merged
merged 1 commit into from
Oct 31, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Oct 29, 2017

Presently I is UniformScaling{Int64}(1), which consistently provides identity behavior in value but inconsistently in type:

julia> typeof(Float32(1)I) # as expected
UniformScaling{Float32}

julia> typeof(Float32(1) + I) # as expected
Float32

julia> typeof(fill(Float32(1), 3, 3) + I) # as expected
Array{Float32,2}

julia> typeof(fill(Float32(1), 3, 3)I) # as expected
Array{Float32,2}

julia> typeof(Int32(1)I) # expect Int32, get Int64?
UniformScaling{Int64}

julia> typeof(Int32(1) + I) # expect Int32, get Int64?
Int64

julia> typeof(fill(Int32(1), 3, 3) + I) # expect eltype Int32, get eltype Int64?
Array{Int64,2}

julia> typeof(fill(Int32(1), 3, 3)I) # expect eltype Int32, get eltype Int64?
Array{Int64,2}

julia> typeof(fill(true, 3, 3)I) # expect eltype Bool, get eltype Int64?
Array{Int64,2}

This pull request makes I yield UniformScaling{Bool}(true) instead, which consistently provides identity behavior not merely in value but also in type. Ref. JuliaLang/LinearAlgebra.jl#454, and particularly the latter paragraphs in JuliaLang/LinearAlgebra.jl#454, which this pull request partially addresses. Best!

@Sacha0 Sacha0 added bugfix This change fixes an existing bug linear algebra Linear algebra labels Oct 29, 2017
@Sacha0 Sacha0 force-pushed the promoteI branch 2 times, most recently from 6b170c0 to 9f71294 Compare October 30, 2017 04:16
Copy link
Member

@fredrikekre fredrikekre left a comment

Choose a reason for hiding this comment

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

Technically breaking since typeof(I) is changing. Perhaps worth a NEWS entry?

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 30, 2017

Technically breaking since typeof(I) is changing. Perhaps worth a NEWS entry?

Good call! Rebased out conflict and added a news entry. Thanks! :)

Given the approvals and 👍s/❤️s from relevant parties, absent objections or requests for time I plan to merge this change this evening or tomorrow morning. Best!

@@ -51,7 +51,7 @@ end
end

@testset "det and logdet" begin
@test det(I) === 1
@test det(I) === true
Copy link
Member

Choose a reason for hiding this comment

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

This is the only bit that gives me pause; I wonder if we should have prod([true,true,true]) === 1 instead of true. We already have sum([true,true,true]) === 3 so it would kind of make sense for the type of prod to match, even though 0 and 1 would be the only possible value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps warrants a dedicated issue? :)

Copy link
Member

Choose a reason for hiding this comment

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

Done: #24425

@Sacha0 Sacha0 merged commit 1536aab into JuliaLang:master Oct 31, 2017
@Sacha0 Sacha0 deleted the promoteI branch October 31, 2017 18:45
@Sacha0
Copy link
Member Author

Sacha0 commented Oct 31, 2017

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants