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

WIP for type-checked static regression #186

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

crherlihy
Copy link
Collaborator

This PR references #183 and is intended to prevent the user from regressing on the dependent variable during feature transformation that may accidentally involve the DV.

The checks are hard-coded right now; need the idiomatic syntax to convert/promote, etc. so that the multi-dispatch on element/col type will work as intended.

@crherlihy crherlihy added the wip label May 8, 2019
@crherlihy crherlihy requested a review from jpfairbanks May 8, 2019 20:50
Copy link
Owner

@jpfairbanks jpfairbanks left a comment

Choose a reason for hiding this comment

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

This is cool, I think we can definitely make it work and be elegant at the same time. I think the biggest thing is to define the new operators with eval (see inline comment), and then to sand down some interface things.

examples/stats_models/regression.jl Outdated Show resolved Hide resolved
examples/stats_models/regression.jl Outdated Show resolved Hide resolved
examples/stats_models/regression.jl Outdated Show resolved Hide resolved
examples/stats_models/regression.jl Show resolved Hide resolved
examples/stats_models/regression.jl Outdated Show resolved Hide resolved
examples/stats_models/regression.jl Outdated Show resolved Hide resolved
end
end

function regress(vec_a::Array{IndepVar{Float64}, 2}, vec_b::Array{IndepVar{Float64}, 2}, orig_x::Array{IndepVar{Float64}, 2}, orig_y::Array{DepVar{Float64}, 2}, op)
Copy link
Owner

Choose a reason for hiding this comment

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

To make these more generic, you can use this construction over Float64:

julia> function foo(a::Vector{T}) where {T<:Number}
         return a .+ 1
         end
foo (generic function with 1 method)

julia> foo([1,2,3])
3-element Array{Int64,1}:
 2
 3
 4

julia> foo([1,2,3.0])
3-element Array{Float64,1}:
 2.0
 3.0
 4.0

Copy link
Owner

Choose a reason for hiding this comment

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

Actual I think this is what they call "triangular dispatch"

Array{T} and then where {T <: DepVar} and then you don't have the constraint that DepVar{U} where U<:Number

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe this is a way to do it

Define a parametric struct

julia> struct Bar{T}
       data::T
       end

# define a method that expects Bar{T<:Number}
julia> function foo(b::Bar{T}) where T<:Number
       return b.data .+ 1
       end
foo (generic function with 2 methods)

# it works, this is the easy case
julia> foo(Bar(1))
2

# it doesn't work if T is not a subtype of number
julia> foo(Bar([1,2]))
ERROR: MethodError: no method matching foo(::Bar{Array{Int64,1}})
Closest candidates are:
  foo(::Array{T<:Number,1}) where T<:Number at REPL[4]:2
  foo(::Bar{T<:Number}) where T<:Number at REPL[10]:2
Stacktrace:
 [1] top-level scope at none:0

# define a case for Bar of Array containing numbers
julia> function foo(b::Bar{Array{T,1}}) where T<:Number
       return b.data .+ 1
       end
foo (generic function with 3 methods)

# this works so nested type constraints are possible.
julia> foo([1,2,3])
3-element Array{Int64,1}:
 2
 3
 4

# now test that the array can be on the outside and the parametric struct can be on the inside.
julia> function foo(b::Array{Bar{T},1}) where T<:Number
       return [x.data .+ 1 for x in b]
       end
foo (generic function with 4 methods)

# success
julia> foo(Bar.([1,2,3]))
3-element Array{Int64,1}:
 2
 3
 4

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So a relatively straightforward fix seems to work unless there's something I've overlooked:

regress(vec_a::Array{IndepVar{T}}, vec_b::Array{DepVar{T}}, orig_x::Array{IndepVar{T}}, orig_y::Array{DepVar{T}}, op) where {T<:Number}

e.g., constrain for the RegComponent parameter type but don't include a constraint that the array dimension parameter <: Integer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants