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

Triangular constructor doesn't get the type #144

Closed
andreasnoack opened this issue Oct 21, 2014 · 20 comments
Closed

Triangular constructor doesn't get the type #144

andreasnoack opened this issue Oct 21, 2014 · 20 comments

Comments

@andreasnoack
Copy link
Member

As noticed in 2e046cd74b3173dc8472f9c57b41ed6482e3044f and JuliaLang/julia#8751, this constructor can't get the UpLo and IsUnit parameters right. E.g.

julia> @code_typed Triangular(randn(4,4), :L, true)
1-element Array{Any,1}:
 :($(Expr(:lambda, {:A,:uplo,:isunit}, {{},{{:A,Array{Float64,2},0},{:uplo,Symbol,0},{:isunit,Bool,0}},{}}, :(begin  # linalg/triangular.jl, line 6:
        chksquare(A::Array{Float64,2})::Int64 # line 7:
        return ((top(apply_type))(Triangular,T,typeof(A::Array{Float64,2})::Type{Array{Float64,2}},uplo::Symbol,isunit::Bool)::Type{_<:Triangular{Float64,Array{Float64,2},UpLo,IsUnit}})(A::Array{Float64,2})::Triangular{Float64,Array{Float64,2},UpLo,IsUnit}
    end::Triangular{Float64,Array{Float64,2},UpLo,IsUnit}))))

Is there something wrong in defining the constructor like this or is the problem somewhere else?

@JeffBezanson
Copy link
Member

It's simply because the compiler only tracks and specializes on argument types, not argument values.

@andreasnoack
Copy link
Member Author

Thank you for explaining it again. I don't know why I didn't realise it here. A while ago I tried to find a solution to almost the exact same problem here https://github.com/andreasnoack/LinearAlgebra.jl/blob/master/src/LinearAlgebra.jl by introducing a dummy parametric immutable for dispatch purposes. Could something like that work?

@andreasnoack andreasnoack changed the title Triangular constructor dones't get the type Triangular constructor doens't get the type Oct 21, 2014
@andreasnoack andreasnoack changed the title Triangular constructor doens't get the type Triangular constructor doesn't get the type Oct 21, 2014
@andreasnoack
Copy link
Member Author

It is really convenient to dispatch on UpLo and IsUnit, but it is not convenient to construct a Triangular in the REPL with Triangular{eltype(A),typeof(A),:L,false}(A), so suggestions are very welcome here.

One solution could be the one I mentioned above where we define a parametrised immutable, say Sym, such that we can do Triagular(A, Sym{:L}, Sym{false}).

Another would be to have four different functions: upperTriangularWithUnitDiagonal, lowerTriangularWithUnitDiagonal, upperTriangularWithoutUnitDiagonal, lowerTriangularWithoutUnitDiagonal.

@JeffBezanson
Copy link
Member

Well, you'd never have to type that in the REPL, because a definition like

Triangular(A) = Triangular{eltype(A),typeof(A),:L,false}(A)

would work equally well.

@andreasnoack
Copy link
Member Author

but you'd want to specify if the matrix is upper or lower and if has unit diagonal.

@JeffBezanson
Copy link
Member

I think a good API would look like Triangular(A, LinAlg.Upper, LinAlg.UnitDiag), and then those constants can be set to whatever is needed, and can be more easily changed in the future.

@timholy
Copy link
Member

timholy commented Oct 21, 2014

Since this comes up a lot, should we put

immutable TypeConst{T} end

into Base? I'm not arguing that LinAlg.Upper and LinAlg.UnitDiag shouldn't exist, simply that there are a lot of cases where you want to pass a value through type inference, and perhaps a general container might be appropriate. If one wanted to use this mechanism here, one could specify lower/upper with TypeConst{:L}/TypeConst{:U} and TypeConst{:UnitDiag}.

@JeffBezanson
Copy link
Member

Yes we should do that. I'd prefer a short name, like Val{:U}. Then we could have

const Upper = Val{:U}()

Not quite sure what to call it though.

@jakebolewski
Copy link
Member

If we switch to using {} for types, couldn't this be {:U}?

@andreasnoack
Copy link
Member Author

Something like that would be great. It would also make the get index methods for factorisations type stable. I also prefer that the name is as short as possible. Does it make a difference if a method dispatches on the type, e.g. foo(Val{:U}) or an instance of the type foo(Val{:U}())?

@JeffBezanson
Copy link
Member

@jakebolewski I haven't thought about tuple types with arbitrary values as elements. It's a problem, since it implies that :U can be its own type ((:U,) is the only sensible instance of {:U,}).

I prefer using Val{x}() and Val{x} to Val{x} and Type{Val{x}}, but it doesn't really make a difference.

@timholy
Copy link
Member

timholy commented Oct 21, 2014

I'm fine with Val. I can't think of anything better, either. If you're satisfied with the name, feel free to proceed with it.

@StefanKarpinski
Copy link
Member

I'm not super keen on the name Val for this.

@timholy
Copy link
Member

timholy commented Oct 21, 2014

TypeVal
Const
Wrap
IVal (inferable value)
IConst

Alternative suggestions welcome.

@StefanKarpinski
Copy link
Member

Is Val{:U} the type that only contains the value :U?

@timholy
Copy link
Member

timholy commented Oct 21, 2014

Val is basically a "type-inferable value". Technically, Val{:U} contains nothing (it's an empty type, it has no fields). Passing Val{false} as a function argument is like passing false, except you can dispatch on it.

@tkelman
Copy link

tkelman commented Oct 21, 2014

How about Dispatch{:U} ? That's what it's for and there isn't anything actually named that in the language yet, right?

@JeffBezanson
Copy link
Member

This is just for creating tokens to dispatch on, not general singleton
types. Adding that is WAY harder.

@andreasnoack
Copy link
Member Author

(:U,) is the only sensible instance of {:U,}

Is it necessary that {:U,} has an instance?

It would really be convenient to use that notation because we all seem to want a short name and consider none of the names appropriate.

@andreasnoack
Copy link
Member Author

Fixed by JuliaLang/julia#9779

@KristofferC KristofferC transferred this issue from JuliaLang/julia Nov 26, 2024
This issue was closed.
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

No branches or pull requests

7 participants