Integer-typed UWDs #921
Integer-typed UWDs #921
Conversation
src/programs/RelationalPrograms.jl
Outdated
| vars -> length(vars) | ||
| else # Typed case. | ||
| elseif typeof(all_types[1]) <: Int # Int typed case |
There was a problem hiding this comment.
you should use eltype(all_types) <: Integer here. Int is a concrete type so it only has itself as subtype. Int32 and Int64 are both subtypes of Integer. The eltype function get the element type of the array, which is more reliable than the type of the first element.
src/programs/RelationalPrograms.jl
Outdated
| elseif typeof(all_types[1]) <: Int # Int typed case | ||
| var_type_map = Dict{Symbol,Int}(zip(all_vars, all_types)) | ||
| vars -> getindex.(Ref(var_type_map), vars) | ||
| elseif typeof(all_types[1]) <: Expr # Expr typed case |
There was a problem hiding this comment.
eltype here too. but I'm thinking you might get a Union{Symbol, Expr} or something like that.
src/programs/RelationalPrograms.jl
Outdated
| else | ||
| error("Context $context mixes typed and untyped variables") | ||
| error("Context $context mixes variable types") |
There was a problem hiding this comment.
Only risk I see here is that when you have expressions, you might want to treat a symbol as the expression containing that symbol. Like how you might want to treat an Int as the list containing only that Int in some contexts. So there might be a valid input that comes out as AbstractVector{Pair{Symbol, Union{Symbol,Expr}}}
test/programs/RelationalPrograms.jl
Outdated
| end | ||
|
|
||
| # Testing doesn't work right now because Julia doesn't like calling n as a function | ||
| # d = RelationDiagram([Expr(n(1)), Expr(n(1)), Expr(n(1))]) |
| # set_junction!(d, [1,4,2,4,3,4]) | ||
| # set_junction!(d, [1,2,3], outer=true) | ||
| # @test parsed == d | ||
|
|
There was a problem hiding this comment.
Mixed Symbol and expression case.
parsed = @relation (x,y,z) where (x::X, y::n(2), z::n(3)+X, w::n(4)) begin
R(x,w)
S(y,w)
T(z,w)
endThis will probably error because of the overly strict typing above.
Mixed symbol, number, expression case:
parsed = @relation (x,y,z) where (x::X, y::n(2), z::n(3)+X, w::4) begin
R(x,w)
S(y,w)
T(z,w)
endThis will definitely error because of the way you are checking element types. Basically the expression case should be handling any subtype of Union{Symbol, Int, Expr} as the element type. And then just think of :foo or :(4) as the expression containing a symbol or number.
src/programs/RelationalPrograms.jl
Outdated
| else # Typed case. | ||
| var_type_map = Dict{Symbol,Symbol}(zip(all_vars, all_types)) | ||
| else | ||
| var_type_map = Dict{Symbol,eltype(all_types)}(zip(all_vars, all_types)) # May have Integer problems |
There was a problem hiding this comment.
oh wait, if you just call Dict without specifying the type parameters, julia will do the right thing.
Dict(zip(all_vars, all_types))
src/programs/RelationalPrograms.jl
Outdated
| vars -> getindex.(Ref(var_type_map), vars) | ||
|
|
src/programs/RelationalPrograms.jl
Outdated
| @@ -194,19 +195,31 @@ function parse_relation_context(context) | |||
| vars = map(terms) do term | |||
| @match term begin | |||
| Expr(:(::), var::Symbol, type::Symbol) => (var => type) | |||
| Expr(:(::), var::Symbol, type::Expr) => (var => type) | |||
|
|
|||
| Expr(:(::), var::Symbol, type) => begin | |||
There was a problem hiding this comment.
you can just add the typecheck for subtype of integer to the match clause.
Expr(:(::), var::Symbol, type::Integer) => ...
src/programs/RelationalPrograms.jl
Outdated
| if vars isa AbstractVector{Symbol} | ||
| (vars, nothing) | ||
| elseif vars isa AbstractVector{Pair{Symbol,Symbol}} | ||
| elseif all(x -> x isa Pair{Symbol, <:Union{Symbol, Integer, Expr}}, vars) |
There was a problem hiding this comment.
easier test is eltype(vars) <: Pair.
I tested it with
julia> eltype([1=>2, :three=>:(2+2), :five=>6]) <: Pair
true| @@ -242,6 +255,7 @@ function parse_relation_inferred_args(args) | |||
| Expr(:kw, name::Symbol, var::Symbol) => (name => var) | |||
| Expr(:(=), name::Symbol, var::Symbol) => (name => var) | |||
| var::Symbol => var | |||
| Expr(:(::), _, _) => error("All variable types must be included in the where clause and not in the argument list") | |||
There was a problem hiding this comment.
Is this a breaking change of behavior? Or just better error message for usage pattern that was already broken? It matters for how we change the version number
There was a problem hiding this comment.
Just a better error message for a usage pattern that was already broken.
|
This is mergable as soon as tests pass! |
Adds additional compatibility to typed UWDs to allow for Int types and Expr types. (Currently, only Symbol types are supported.)