det(I) #23364

ChrisRackauckas · 2017-08-20T03:48:00Z

The determinant for the UniformScaling operator was missing.

musm · 2017-08-20T04:15:16Z

base/linalg/uniformscaling.jl

@@ -167,6 +167,7 @@ end

 inv(J::UniformScaling) = UniformScaling(inv(J.λ))
 norm(J::UniformScaling, p::Real=2) = abs(J.λ)
+det(J::UniformScaling) = 1.0


Actually I don't think this handles correctly when \lambda is not 1

yeah that's better.

ararslan · 2017-08-20T04:32:51Z

Could also add logdet(::UniformScaling{T}) where {T} = log(one(T))

ChrisRackauckas · 2017-08-20T04:35:08Z

I made it zero(T) instead.

musm · 2017-08-20T04:35:58Z

base/linalg/uniformscaling.jl

@@ -167,6 +167,8 @@ end

 inv(J::UniformScaling) = UniformScaling(inv(J.λ))
 norm(J::UniformScaling, p::Real=2) = abs(J.λ)
+det(J::UniformScaling{T}) where {T} = one(T)


Right, but det (10*eye(n,n)) == 10^n not 1.0, so I don't think this will work.

oh wait, forgot about the lambda. My bad.

I guess you could check if \lambda == 1 or else throw, but doesn't seem very useful ?

Well, it is useful if you don't want to special case it. I opened this PR because I ran into that exact issue:

https://github.com/JuliaDiffEq/DiffEqBase.jl/blob/master/src/problems/dde_problems.jl#L36

ararslan · 2017-08-20T04:38:18Z

zero(T) isn't right; typeof(log(det(eye(Int, 3, 3)))) is Float64.

ararslan · 2017-08-20T04:46:21Z

base/linalg/uniformscaling.jl

@@ -167,6 +167,8 @@ end

 inv(J::UniformScaling) = UniformScaling(inv(J.λ))
 norm(J::UniformScaling, p::Real=2) = abs(J.λ)
+det(J::UniformScaling{T}) where {T} = J.λ==one(T) ? one(T) : error("Determinant of UniformScaling is only well-defined when λ=1")


ArgumentError would be better than error

musm · 2017-08-20T04:49:30Z

base/linalg/uniformscaling.jl

@@ -167,6 +167,8 @@ end

 inv(J::UniformScaling) = UniformScaling(inv(J.λ))
 norm(J::UniformScaling, p::Real=2) = abs(J.λ)
+det(J::UniformScaling{T}) where {T} = J.λ==one(T) ? one(T) : ArgumentError("Determinant of UniformScaling is only well-defined when λ=1")


isone(J.λ) ? one(T) : throw(ArgumentError("Determinant of UniformScaling is only well-defined when λ = 1."))

ararslan · 2017-08-20T05:09:01Z

base/linalg/uniformscaling.jl

@@ -167,6 +167,8 @@ end

 inv(J::UniformScaling) = UniformScaling(inv(J.λ))
 norm(J::UniformScaling, p::Real=2) = abs(J.λ)
+det(J::UniformScaling{T}) where {T} = isone(J.λ) ? one(T) : throw(ArgumentError("Determinant of UniformScaling is only well-defined when λ=1."))


Actually one is wrong as well. det(eye(Int, 3, 3)) is Float64, whereas det(I) will be Int.

det(eye(Int, 3, 3)) is a Float64 only because it is promoted to perform an LU factorization. In this case we don't need that so I think one(T) is perfectly fine. We have for example:

julia> det(Diagonal([1,2,3])) 6

for the same reason; no need to promote for LU.

0I should also be fine?

fredrikekre · 2017-08-20T10:09:22Z

base/linalg/uniformscaling.jl

@@ -167,6 +167,8 @@ end

 inv(J::UniformScaling) = UniformScaling(inv(J.λ))
 norm(J::UniformScaling, p::Real=2) = abs(J.λ)
+det(J::UniformScaling{T}) where {T} = isone(J.λ) ? one(T) : throw(ArgumentError("Determinant of UniformScaling is only well-defined when λ=1."))
+logdet(::UniformScaling{T}) where {T} = isone(J.λ) ? log(one(T)) : throw(ArgumentError("Determinant of UniformScaling is only well-defined when λ=1."))


Missing J for the argument. But we actually do not need this since we have

julia/base/linalg/generic.jl

Line 1267 in e701561

logdet(A) = log(det(A))

codecov-io · 2017-08-20T12:46:59Z

Codecov Report

Merging #23364 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #23364      +/-   ##
==========================================
- Coverage   54.98%   54.98%   -0.01%     
==========================================
  Files         275      275              
  Lines       51130    51132       +2     
==========================================
- Hits        28115    28114       -1     
- Misses      23015    23018       +3

Impacted Files	Coverage Δ
base/twiceprecision.jl	`57.79% <100%> (+0.16%)`	⬆️
base/distributed/managers.jl	`16.75% <0%> (-2.24%)`	⬇️
base/range.jl	`49.37% <0%> (-0.13%)`	⬇️
base/printf.jl	`60.3% <0%> (+0.15%)`	⬆️
base/channels.jl	`47.89% <0%> (+0.84%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c54e7ac...241c3bf. Read the comment docs.

giordano · 2017-08-20T14:43:38Z

zero(T) isn't right; typeof(log(det(eye(Int, 3, 3)))) is Float64.

What about zero(float(T))? Anyway, the case pointed out by @ararslan is be a good test to include.

ChrisRackauckas · 2017-08-20T20:15:21Z

Why should it match eye in type? I don't see why a priori that should be the case. If it can return an Int that's better?

fredrikekre · 2017-08-20T20:38:38Z

I think the current implementation is OK. There is a trailing whitespace (which fails CI) and would be great with some testcases :)

ararslan · 2017-08-22T05:49:59Z

test/linalg/uniformscaling.jl

+ @test typeof(det(I)) == Int
+ @test typeof(det(1.0I)) == Float64
+ @test iszero(det(0I))
+ @test isone(logdet(I))


This should be iszero, shouldn't it?

fredrikekre

🇸🇪

fredrikekre · 2017-08-22T08:54:10Z

test/linalg/uniformscaling.jl

@@ -43,6 +43,15 @@ end
 @test UniformScaling(α)./α == UniformScaling(1.0)
 end

+@testset "det and logdet" begin
+ @test isone(det(I))


Perhaps we should actually change this to @test det(I) === 1 instead. Since isone(I) is true, this will still succeed if det(::UniformScaling) becomes a no-op :)

@ov

When an input line like `[import|using] AAA: xxx` is entered, if `AAA` is already loaded in the REPL session, it will now autocomplete to the names available in `AAA` that start with `xxx`. For example, it will autocomplete `using Base.Experimental: @ov|` to `using Base.Experimental: @overlay`. - fixes #23364

@ov

When an input line like `[import|using] AAA: xxx` is entered, if `AAA` is already loaded in the REPL session, it will now autocomplete to the names available in `AAA` that start with `xxx`. For example, it will autocomplete `using Base.Experimental: @ov|` to `using Base.Experimental: @overlay`. - fixes #23364

@ov

When an input line like `[import|using] AAA: xxx` is entered, if `AAA` is already loaded in the REPL session, it will now autocomplete to the names available in `AAA` that start with `xxx`. For example, it will autocomplete `using Base.Experimental: @ov|` to `using Base.Experimental: @overlay`. - fixes #23364

@ov

When an input line like `[import|using] AAA: xxx` is entered, if `AAA` is already loaded in the REPL session, it will now autocomplete to the names available in `AAA` that start with `xxx`. For example, it will autocomplete `using Base.Experimental: @ov|` to `using Base.Experimental: @overlay`. - fixes #23364

@ov

When an input line like `[import|using] AAA: xxx` is entered, if `AAA` is already loaded in the REPL session, it will now autocomplete to the names available in `AAA` that start with `xxx`. For example, it will autocomplete `using Base.Experimental: @ov|` to `using Base.Experimental: @overlay`. - fixes #23364

det(I)

ba3fab6

The determinant for the UniformScaling operator was missing.

musm reviewed Aug 20, 2017

View reviewed changes

Update uniformscaling.jl

ed6b3c5

ararslan added domain:linear algebra Linear algebra needs tests Unit tests are required for this change labels Aug 20, 2017

Update uniformscaling.jl

6937aa6

musm reviewed Aug 20, 2017

View reviewed changes

Update uniformscaling.jl

a187383

ararslan reviewed Aug 20, 2017

View reviewed changes

Update uniformscaling.jl

34d27ed

musm reviewed Aug 20, 2017

View reviewed changes

Update uniformscaling.jl

241c3bf

ararslan reviewed Aug 20, 2017

View reviewed changes

fredrikekre reviewed Aug 20, 2017

View reviewed changes

Update uniformscaling.jl

fabbfc8

ChrisRackauckas added 2 commits August 20, 2017 14:51

Update uniformscaling.jl

21acf21

Add UniformScaling det tests

6388641

ararslan reviewed Aug 22, 2017

View reviewed changes

fredrikekre approved these changes Aug 22, 2017

View reviewed changes

ararslan removed the needs tests Unit tests are required for this change label Aug 22, 2017

ChrisRackauckas and others added 2 commits August 21, 2017 22:51

Update uniformscaling.jl

5968213

Use a long form definition to keep line length down

534fef4

ararslan approved these changes Aug 22, 2017

View reviewed changes

fredrikekre reviewed Aug 22, 2017

View reviewed changes

update tests

f32079c

andreasnoack merged commit bc66f02 into JuliaLang:master Aug 22, 2017

ChrisRackauckas deleted the patch-7 branch August 22, 2017 14:09

aviatesk mentioned this pull request Jun 6, 2024

REPLCompletions: allow completions for [import|using] AAA: xxx #54719

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

det(I) #23364

det(I) #23364

ChrisRackauckas commented Aug 20, 2017

musm Aug 20, 2017 •

edited

Loading

ChrisRackauckas Aug 20, 2017

ararslan commented Aug 20, 2017

ChrisRackauckas commented Aug 20, 2017

musm Aug 20, 2017 •

edited

Loading

ChrisRackauckas Aug 20, 2017

musm Aug 20, 2017

ChrisRackauckas Aug 20, 2017

ararslan commented Aug 20, 2017

ararslan Aug 20, 2017

musm Aug 20, 2017

ararslan Aug 20, 2017

fredrikekre Aug 20, 2017

yuyichao Aug 20, 2017

fredrikekre Aug 20, 2017

codecov-io commented Aug 20, 2017

giordano commented Aug 20, 2017

ChrisRackauckas commented Aug 20, 2017

fredrikekre commented Aug 20, 2017

ararslan Aug 22, 2017

ChrisRackauckas Aug 22, 2017

fredrikekre left a comment

fredrikekre Aug 22, 2017

det(I) #23364

det(I) #23364

Conversation

ChrisRackauckas commented Aug 20, 2017

musm Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Aug 20, 2017

ChrisRackauckas commented Aug 20, 2017

musm Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Aug 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 20, 2017

Codecov Report

giordano commented Aug 20, 2017

ChrisRackauckas commented Aug 20, 2017

fredrikekre commented Aug 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musm Aug 20, 2017 •

edited

Loading

musm Aug 20, 2017 •

edited

Loading