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

lu(A, Val{false}) should be lu(A, pivot=false) #476

Closed
stevengj opened this issue Oct 14, 2017 · 9 comments · Fixed by JuliaLang/julia#40623
Closed

lu(A, Val{false}) should be lu(A, pivot=false) #476

stevengj opened this issue Oct 14, 2017 · 9 comments · Fixed by JuliaLang/julia#40623

Comments

@stevengj
Copy link
Member

stevengj commented Oct 14, 2017

I use the unpivoted lu function a lot in teaching linear algebra, because its results are more easily compared to hand calculations (where you rarely permute rows), and it is continually annoying to me that you do this by passing Val{false} as the second argument — I feel like I'm exposing an ugly internal. Can't we changed this to a pivot=false keyword?

LU factorization is a fairly expensive operation, so I am mystified by why we would care about dispatch performance here. Surely the cost of a runtime branch is negligible?

cc @andreasnoack

@andreasnoack
Copy link
Member

andreasnoack commented Oct 15, 2017

It's for consistency with the other factorizations that support pivoting (Cholesky and QR) because they have special types and we therefore need compile time dispatch for type stability in those cases. I don't like Vals but I wouldn't like different kinds of pivoting arguments for different factorizations. I'd be happy with finding a prettier alternative as long as it is consistent across factorizations and doesn't sacrifice type stability.

@stevengj
Copy link
Member Author

stevengj commented Oct 15, 2017

Maybe define a singleton type? e.g. lu(A, NoPivot) or lu(A, Pivot{false})?

@oscardssmith
Copy link
Member

I have no idea if this is feasible, but would it be possible to combine keyword arguments and Vals, to make it be pivot = Val{false}? Alternatively, is there any reason Val is needed for dispatch on the caller side? It feels like the caller should just pass false, and then internals should be able to figure out if there are multiple methods depending on the value passed in.

@stevengj
Copy link
Member Author

stevengj commented Oct 15, 2017

@oscardssmith, the whole difficulty here is that checking the values can only be done at runtime, not a compile time. This is too late for type-stability if the return type depends on whether pivot == true, as it does for qrfact:

julia> qrfact(rand(5,3), Val{true})
Base.LinAlg.QRPivoted{Float64,Array{Float64,2}} with factors Q and R:
[-0.33213 -0.66443 … -0.454553 -0.484674; -0.153278 -0.529465 … 0.0638774 0.805849; … ; -0.594088 -0.104926 … 0.743544 -0.183901; -0.413672 0.370117 … -0.0415064 -0.0449306]
[-1.65013 -1.12184 -0.475066; 0.0 -0.803798 -0.0116616; 0.0 0.0 0.529326]

julia> qrfact(rand(5,3), Val{false})
Base.LinAlg.QRCompactWY{Float64,Array{Float64,2}} with factors Q and R:
[-0.0727649 0.501108 … -0.087708 -0.694424; -0.240891 -0.318195 … -0.65019 0.295001; … ; -0.468053 -0.507519 … 0.628181 -0.165707; -0.82135 0.187781 … -0.245073 -0.0892836]
[-0.618574 -0.833208 -1.18345; 0.0 0.580143 -0.202586; 0.0 0.0 -0.47824]

@Sacha0
Copy link
Member

Sacha0 commented Oct 15, 2017

(Closely related to JuliaLang/julia#24011.)

@andreasnoack
Copy link
Member

Ref: JuliaLang/julia#22232

@ChrisRackauckas
Copy link
Member

Since 1.0 will have JuliaLang/julia#24362 , we should consider getting rid of the value-type dispatching here and simplifying the API to true/false which can be type-stable on literal arguments when/if it's propagated. This would be a breaking API change, so it would have to be 1.0?

@martinholters
Copy link
Member

This would be a breaking API change, so it would have to be 1.0?

We could still allow a Val to be passed, to make the change non-breaking.

@alanedelman
Copy link
Contributor

I hate LU(A,Val{False}) :-(

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 a pull request may close this issue.

7 participants