Make `allowrankdeficient` a keyword argument #386

pdeffebach · 2020-11-01T19:26:41Z

In reference to #351.

Whether or not lm should allow a rank deficient matrix is an option, and true or false is not an input value on its own. Therefore it makes sense that this be a keyword argument and not a positional argument.

The docs are also improved.

No changes to glm because I don't think any aspect of glm allows rank deficient input matrices, even though it probably should. But thats another PR.

pdeffebach · 2020-11-03T13:14:19Z

Tests fail for unrelated reasons. I will try and figure out and fix with another PR.

This is ready for a review.

src/lm.jl

…/GLM.jl into allowrankdeficient_kw

pdeffebach · 2020-11-05T03:40:50Z

Tests pass now, the "trim trailing whitespace" option in Sublime text was breaking the printing tests.

Ready for a review.

kleinschmidt

LGTM, other than a few comments on the tests

kleinschmidt · 2020-11-05T19:37:36Z

test/runtests.jl

                   4.108734932819495, 4.995249696954908, 6.075962907632594, 0.0, 8.038151489191618,
                   8.848886704358202, 2.8697881579099085, 11.15107375630744, 11.8392578374927])
+
+    m2p_dep = fit(LinearModel, Xmissingcell, ymissingcell, true)


maybe add a test that this warns?

and I guess maybe also the behavior when the kwarg and the positional conflict?

kleinschmidt · 2020-11-05T19:39:16Z

src/glmfit.jl

 end

-function GlmResp(y::AbstractVector{<:Real}, d::D, l::L, off::AbstractVector{<:Real}, 
+function GlmResp(y::AbstractVector{<:Real}, d::D, l::L, off::AbstractVector{<:Real},


can you revert all these whitespace changes so that the git blame is clean?

nalimilan · 2020-11-07T14:48:13Z

src/lm.jl

+    if allowrankdeficient_dep != nothing
+        @warn "Positional argument `allowrankdeficient` is deprecated, use keyword" *
+              "argument instead. Proceeding with positional argument  value: $allowrankdeficient_dep"
+              allowrankdeficient = allowrankdeficient_dep


Suggested change

if allowrankdeficient_dep != nothing

@warn "Positional argument `allowrankdeficient` is deprecated, use keyword" *

"argument instead. Proceeding with positional argument value: $allowrankdeficient_dep"

allowrankdeficient = allowrankdeficient_dep

if allowrankdeficient_dep != nothing

Base.depwarn("Positional argument `allowrankdeficient` is deprecated, use keyword" *

"argument instead. Proceeding with positional argument value: $allowrankdeficient_dep",

:fit)

allowrankdeficient = allowrankdeficient_dep

I couldn't get this to work, I'll try again in a bit.

Got it to work!

codecov-io · 2020-11-07T15:53:17Z

Codecov Report

Merging #386 (ae7669e) into master (3234f5e) will increase coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #386      +/-   ##
==========================================
+ Coverage   79.56%   79.79%   +0.23%     
==========================================
  Files           7        7              
  Lines         685      688       +3     
==========================================
+ Hits          545      549       +4     
+ Misses        140      139       -1

Impacted Files	Coverage Δ
src/lm.jl	`93.33% <100.00%> (-0.92%)`	⬇️
src/glmfit.jl	`74.89% <0.00%> (-2.47%)`	⬇️
src/linpred.jl	`70.70% <0.00%> (+8.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3234f5e...5aa77bd. Read the comment docs.

src/lm.jl

pdeffebach · 2020-11-12T15:38:54Z

Thanks for the reviews. I responded to all comments.

Ready for another round!

nalimilan · 2020-11-13T08:35:08Z

By making this a keyword argument, the name allowrankdeficient becomes part of the API, so I think it's worth considering whether it's the best possible name. I must say I don't find it very user-friendly, but I don't have very good suggestions either. Are there examples of similar options in other software?

src/lm.jl

test/runtests.jl

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

pdeffebach · 2020-11-14T21:29:20Z

I also don't have any good suggestions. Stata and R do it by default, while it looks like python doesn't allow it at all.

Would it be too breaking to make it true by default?

nalimilan · 2020-11-14T21:50:14Z

AFAICT it wouldn't be breaking at all since it would turn an error into something that succeeds.

@dmbates @andreasnoack Opinions?

pdeffebach · 2020-11-26T15:17:35Z

Okay just pushed a commit to

Rename allowrankdeficient to dropcollinear
Make the default of collinear to true. This will cause fewer "what's going on?" scenarios for new users who are less familiar with the math behind OLS.

It also has an added performance benefit!

julia> x = rand(10000, 50);

julia> @btime GLM.cholpred(x, true);
  2.134 ms (17 allocations: 7.67 MiB)

julia> @btime GLM.cholpred(x, true);
  2.133 ms (17 allocations: 7.67 MiB)

julia> @btime GLM.cholpred($x, true);
  2.130 ms (17 allocations: 7.67 MiB)

julia> @btime GLM.cholpred($x, false);
  3.031 ms (15 allocations: 7.67 MiB)

julia> @btime GLM.cholpred($x, false);
  3.029 ms (15 allocations: 7.67 MiB)

Tests are updated. I haven't added anything to docs/ as I'm not sure where it will go. But the docstring is updated.

src/lm.jl

test/runtests.jl

pdeffebach · 2020-11-26T15:46:35Z

Ready for a review!

test/runtests.jl

src/lm.jl

pdeffebach · 2020-12-18T17:18:28Z

Bumping this! Just fixed some errant white space changes.

To recap: Changes allowrankdeficient to a keyword argument and the new name is dropcollinear

kleinschmidt · 2020-12-18T19:52:22Z

LGTM. Nightly failures are due to the p-value printing thing with printfpocalypse (so unrelated)

nalimilan

Looks good! Just a wording suggestion.

src/lm.jl

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

pdeffebach

Thanks!

pdeffebach added 2 commits November 1, 2020 14:24

initial commit

0288828

remove manifest

098a8f2

pdeffebach commented Nov 3, 2020

View reviewed changes

src/lm.jl Outdated Show resolved Hide resolved

pdeffebach and others added 6 commits November 3, 2020 08:15

Update src/lm.jl

1874153

fix whitespace

48f47be

Merge branch 'allowrankdeficient_kw' of https://github.com/pdeffebach…

ca7426c

…/GLM.jl into allowrankdeficient_kw

delete manifest

f0a4928

tests pass

ce6ad79

delete manifest

9965c01

kleinschmidt requested changes Nov 5, 2020

View reviewed changes

nalimilan reviewed Nov 7, 2020

View reviewed changes

fix tests

162a6f9

pdeffebach added 2 commits November 7, 2020 10:58

fix whitespace

cc86d17

fix whitespace 2

0ba3b46

pdeffebach commented Nov 7, 2020

View reviewed changes

src/lm.jl Outdated Show resolved Hide resolved

Update src/lm.jl

a03d4b9

kleinschmidt approved these changes Nov 12, 2020

View reviewed changes

nalimilan reviewed Nov 13, 2020

View reviewed changes

Apply suggestions from code review

af75825

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

change name to collinear, make default true

8f2e9f2

pdeffebach commented Nov 26, 2020

View reviewed changes

whitespace fixes

6057771

pdeffebach commented Dec 18, 2020

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

pdeffebach commented Dec 18, 2020

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

pdeffebach added 2 commits December 18, 2020 12:16

Update test/runtests.jl

cedba99

Update test/runtests.jl

ae7669e

pdeffebach commented Dec 18, 2020

View reviewed changes

src/lm.jl Outdated Show resolved Hide resolved

Update src/lm.jl

5aa77bd

kleinschmidt approved these changes Dec 18, 2020

View reviewed changes

nalimilan approved these changes Dec 18, 2020

View reviewed changes

src/lm.jl Outdated Show resolved Hide resolved

Update src/lm.jl

6dacb55

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

pdeffebach commented Dec 18, 2020

View reviewed changes

nalimilan merged commit 9505791 into JuliaStats:master Dec 28, 2020

nalimilan mentioned this pull request Mar 20, 2021

buggy deprecation for :confint interval #408

Closed

pdeffebach mentioned this pull request Apr 29, 2021

Incorrect linear regression results #426

Closed

andreasnoack mentioned this pull request Nov 19, 2024

Validation, CI, Residual variance, compare with lm() in R #280

Closed

Make allowrankdeficient a keyword argument #386

Make allowrankdeficient a keyword argument #386

Uh oh!

Conversation

pdeffebach commented Nov 1, 2020

Uh oh!

pdeffebach commented Nov 3, 2020

Uh oh!

Uh oh!

pdeffebach commented Nov 5, 2020

Uh oh!

kleinschmidt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Nov 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

pdeffebach commented Nov 12, 2020

Uh oh!

nalimilan commented Nov 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdeffebach commented Nov 14, 2020

Uh oh!

nalimilan commented Nov 14, 2020

Uh oh!

pdeffebach commented Nov 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdeffebach commented Nov 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdeffebach commented Dec 18, 2020

Uh oh!

kleinschmidt commented Dec 18, 2020

Uh oh!

nalimilan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pdeffebach left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Make `allowrankdeficient` a keyword argument #386

Make `allowrankdeficient` a keyword argument #386

codecov-io commented Nov 7, 2020 •

edited

Loading