-
Notifications
You must be signed in to change notification settings - Fork 114
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
Handle rank-deficient LinearModel. Also changes in docs #207
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for new docs!
src/GLM.jl
Outdated
@@ -76,8 +76,18 @@ module GLM | |||
const FP = AbstractFloat | |||
const FPVector{T<:FP} = AbstractArray{T,1} | |||
|
|||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong indentation. Same below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the correct indentation in this case? I thought that the first line within the multi-line string should be indented 4 spaces (b/c Markdown) and the line beginning with "Abstract" should not be indented. If I indent the triple double-quote characters things will look weird.
Perhaps the best solution is to move those abstract type declarations to linpred.jl and lm.jl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you can use whatever indentation you want (i.e. the same as surrounding lines), what matters is the alignment with the """
.
src/glmfit.jl
Outdated
@@ -54,6 +54,8 @@ When `L` is the canonical link for `D` the derivative of the inverse link is a m | |||
of the variance function for `D`. If they are the same a numerator and denominator term in | |||
the expression for the working weights will cancel. | |||
""" | |||
function cancancel end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to attach the docstring to the method below? If that's because it doesn't appear in the manual, I think you to list argument types there, i.e. cancancel(::GlmResp)
.
test/runtests.jl
Outdated
@testset "rankdeficient" begin | ||
# an example of rank deficiency caused by a missing cell in a table | ||
dfrm = DataFrame([categorical(repeat(string.('A':'D'), inner = 6)), | ||
categorical(repeat(string.('a':'c'), inner = 2, outer = 4))], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect alignment.
test/runtests.jl
Outdated
ymissingcell = y[inds] | ||
@test_throws LinAlg.PosDefException m2 = fit(LinearModel, Xmissingcell, ymissingcell) | ||
m2p = fit(LinearModel, Xmissingcell, ymissingcell, true) | ||
@test isapprox(deviance(m2p), 0.2859221258731563) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make sense to test the values of coefficients?
|
||
makedocs() | ||
makedocs( | ||
format = :html, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove docs/mkdocs.yml
with that option. And of course the old docs/
folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks.
For reasons that I am having difficulty remembering, I incorporated documentation changes in these changes to handle rank-deficient linear models as discussed in #200.
I realize this makes it harder to review these changes for rank-deficiency. I can start a new branch and cherry-pick the rank-deficiency-related commits if that would be preferred.