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

Stackoverflow with views #444

Closed
tlienart opened this issue Aug 17, 2021 · 7 comments
Closed

Stackoverflow with views #444

tlienart opened this issue Aug 17, 2021 · 7 comments

Comments

@tlienart
Copy link
Contributor

tlienart commented Aug 17, 2021

This was reported by an MLJ user here and after some digging it seems that an issue is to do with how a vector y is passed to GLM's glm method; GLM will crash if y is a view. Here's a reproducing example by @olivierlabayle; note that the use of MLJ is just for the sake of getting the same data, everything else is GLM

julia> using Distributions, GLM, MLJ

julia> X, y = @load_boston;

julia> Xmatrix = MLJ.matrix(X);

julia> typeof(X)
NamedTuple{(:Crim, :Zn, :Indus, :NOx, :Rm, :Age, :Dis, :Rad, :Tax, :PTRatio, :Black, :LStat), NTuple{12, SubArray{Float64, 1, Matrix{Float64}, Tuple{Base.Slice{Base.OneTo{Int64}}, Int64}, true}}}

julia> typeof(y)
SubArray{Float64, 1, Matrix{Float64}, Tuple{Base.Slice{Base.OneTo{Int64}}, Int64}, true}

julia> typeof(Xmatrix)
Matrix{Float64} (alias for Array{Float64, 2})

and now this is fine (note the convert)

julia> GLM.glm(Xmatrix, convert(Vector, y), Distributions.Normal(), GLM.IdentityLink())
GeneralizedLinearModel{GLM.GlmResp{Vector{Float64}, Normal{Float64}, IdentityLink}, GLM.DensePredChol{Float64, LinearAlgebra.Cholesky{Float64, Matrix{Float64}}}}:

Coefficients:
──────────────────────────────────────────────────────────────────────
           Coef.  Std. Error      z  Pr(>|z|)   Lower 95%    Upper 95%
──────────────────────────────────────────────────────────────────────
x1   -0.0981569   0.0346913   -2.83    0.0047  -0.166151   -0.0301631
x2    0.0494157   0.0145318    3.40    0.0007   0.020934    0.0778975
x3    0.0166277   0.064685     0.26    0.7971  -0.110153    0.143408
x4   -2.25577     3.38342     -0.67    0.5050  -8.88717     4.37562
x5    5.99805     0.311101    19.28    <1e-82   5.3883      6.60779
x6   -0.00514634  0.0139229   -0.37    0.7117  -0.0324346   0.022142
x7   -0.972546    0.197394    -4.93    <1e-06  -1.35943    -0.585661
x8    0.193115    0.0669889    2.88    0.0039   0.0618192   0.324411
x9   -0.01087     0.00393042  -2.77    0.0057  -0.0185735  -0.00316651
x10  -0.425742    0.110342    -3.86    0.0001  -0.642009   -0.209475
x11   0.015433    0.00271568   5.68    <1e-07   0.0101104   0.0207556
x12  -0.424929    0.051171    -8.30    <1e-15  -0.525222   -0.324635
──────────────────────────────────────────────────────────────────────

but this isn't:

julia> GLM.glm(Xmatrix, y, Distributions.Normal(), GLM.IdentityLink())
ERROR: StackOverflowError:
Stacktrace:
 [1] GLM.GlmResp(y::SubArray{Float64, 1, Matrix{Float64}, Tuple{Base.Slice{Base.OneTo{Int64}}, Int64}, true}, d::Normal{Float64}, l::IdentityLink, off::Vector{Float64}, wts::Vector{Float64}) (repeats 79984 times)
   @ GLM ~/.julia/packages/GLM/5CcRd/src/glmfit.jl:63

The culprit is GlmResp that doesn't seem to dispatch properly and loops to infinity.

(bug) pkg> status
      Status `~/Desktop/jsbx/bug/Project.toml`
  [31c24e10] Distributions v0.25.11
  [38e38edf] GLM v1.5.1
  [add582a8] MLJ v0.16.7

cc @olivierlabayle, @ablaom, @goerch

@nalimilan
Copy link
Member

Looks like we need a fix similar to #443.

@ablaom
Copy link
Contributor

ablaom commented Oct 14, 2021

@bkamins @nalimilan Been revisiting this issue and wondered if there was anyone with time to push this along?

@bkamins
Copy link
Contributor

bkamins commented Oct 14, 2021

I think - assuming he has time - @andreasnoack had spent most time thinking about how to fix these things.

@andreasnoack
Copy link
Member

@ablaom Does #446 fix that issue?

@ablaom
Copy link
Contributor

ablaom commented Oct 14, 2021

@jbrea would you have a chance to check this? This concerns JuliaAI/MLJGLMInterface.jl#10 .

@jbrea
Copy link

jbrea commented Nov 1, 2021

Not sure I'm the right person to look at this. But I checked the following:

(@v1.6) pkg> activate --temp

(jl_Aj9lB4) pkg> add Distributions, MLJ, GLM#6059527

julia> using Distributions, GLM, MLJ

julia> X, y = @load_boston;

julia> Xmatrix = MLJ.matrix(X);

julia> GLM.glm(Xmatrix, y, Distributions.Normal(), GLM.IdentityLink())

which runs without errors on julia 1.6.3.

@ablaom
Copy link
Contributor

ablaom commented Nov 1, 2021

@jbrea Thanks for checking! I'm so sorry, I pasted the wrong handle 🙄 .

@andreasnoack It looks like we can close this now, thanks.

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

6 participants