Allow users to use integer arrays as input (but convert them). #130
Conversation
| @@ -64,7 +64,7 @@ end | |||
| cholfact(x::LinearModel) = cholfact(x.pp) | |||
|
|
|||
| function fit{LmRespT<:LmResp,LinPredT<:LinPred}(::Type{LinearModel{LmRespT,LinPredT}}, | |||
| X::AbstractMatrix, y::Vector) | |||
| X::@compat(Union{Matrix,SparseMatrixCSC{FP}}), y::FPVector) | |||
There was a problem hiding this comment.
SparseMatrixCSC{FP} will only accept matrices with the abstract type FP, and not of any concrete subtypes. Use T<:FP as for GLMs.
BTW, it would be great to test sparse matrices (which would have caught this problem).
|
Thanks! I've made some comments. |
|
Great comments, I will address them, and add tests for sparse matrices. |
|
Is this better? Notice I'm doing |
|
I don't like calling |
| yy = convert(Vector{T}, y) | ||
| return fit(LinearModel{LmResp{typeof(yy)}, DensePredQR{T}}, X, y) | ||
| function fit(::Type{LinearModel}, X::@compat(Union{Matrix,SparseMatrixCSC}), y::Vector) | ||
| y = float(y) |
There was a problem hiding this comment.
For type stability, assign the result to yy (even if here the function is pretty short so it shouldn't make a big difference).
|
Alright, I'll let it fail instead. Yes it works for |
…test with sparse X.
| function fit{LmRespT<:LmResp,LinPredT<:LinPred}(::Type{LinearModel{LmRespT,LinPredT}}, | ||
| X::AbstractMatrix, y::Vector) | ||
| function fit{LmRespT<:LmResp,LinPredT<:LinPred, T<:FP}(::Type{LinearModel{LmRespT,LinPredT}}, | ||
| X::@compat(Union{Matrix{T},SparseMatrixCSC{T}}), y::FPVector) |
There was a problem hiding this comment.
Why restrict the matrix type?
There was a problem hiding this comment.
I just followed the glm code.
There was a problem hiding this comment.
If AbstractMatrix doesn't cause any issues, I'd stick with that here and change glm instead (in a separate PR).
There was a problem hiding this comment.
Sure, I'll change it later.
|
I've "reverted" to AbstractMatrix. Notice the parametric type in the "first" |
Allow users to use integer arrays as input (but convert them).
|
Thanks |
Fixes #84 .