search

JuliaStats / GLMNet.jl

Julia wrapper for fitting Lasso/ElasticNet GLM models using glmnet
Other
96 stars 35 forks source link

Multinomial and Cox redux; also fixing showarray error #50

Closed dsweber2 closed 3 years ago

dsweber2 commented 3 years ago

This is a minor rewrite of pull request https://github.com/JuliaStats/GLMNet.jl/pull/10 so that it actually works with the recent version of Julia. I dropped the gadfly plots and dependency, as that would be better done using Recipes in the current Julia ecosystem. I added @testsets to demarcate testign the different models. In the process of all this I ran into a usage of the deprecated showarray, so I also fixed that.

codecov-io commented 3 years ago

Codecov Report

Merging #50 (0506b46) into master (456b9ae) will increase coverage by 10.20%. The diff coverage is 91.98%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #50       +/-   ##
===========================================
+ Coverage   83.33%   93.53%   +10.20%     
===========================================
  Files           1        3        +2     
  Lines         240      464      +224     
===========================================
+ Hits          200      434      +234     
+ Misses         40       30       -10
Impacted Files Coverage Δ
src/CoxNet.jl 90.17% <90.17%> (ø)
src/Multinomial.jl 93.00% <93.00%> (ø)
src/GLMNet.jl 95.23% <96.00%> (+11.90%) :arrow_up:

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 456b9ae...0506b46. Read the comment docs.

dsweber2 commented 3 years ago

I appreciate your quick turn-around time.

the duplication of the GLMNetPath struct, which could probably have a type parameter added instead so that it can reuse some of the generic functions

this was easy enough to hack in through parametric types. There shouldn't be too many of them spawned by this, so it seemed a good use.

the duplication of the glmnetcv function for cox duplication of the some of the generic functions like predict

I was under the impression that if they're sufficiently different in execution, using the type dispatch was the correct approach. I didn't really check if they could be integrated cleanly though, so I was mostly relying on the judgement of the original PR.

Another quick question I had, do we know if the results for the added tests line up with the results that R gives? It would be good to double check this if possible

I haven't double checked it, and was assuming that the hand entered values were from R. I don't currently have glmnet working in R (part of the reason I hacked this together in the first place)

dsweber2 commented 3 years ago

CategoricalArray was causing the 1.0 tests to break. I ditched it by getting more specific with the types in GLMNet.jl's copy of glmnet and making the y in Multinomial.jl generic. This didn't cause any tests to break, but it's not the most elegant solution.

I'm not sure what's going on with the broken tests now; check_jerr only has two arguments? the error claims it only has three...

JackDunnNZ commented 3 years ago

Thanks for the changes, I think this is good to go once we get tests passing. I'm happy to trust that the magic numbers in the test make sense!

I was under the impression that if they're sufficiently different in execution, using the type dispatch was the correct approach. I didn't really check if they could be integrated cleanly though, so I was mostly relying on the judgement of the original PR.

The only reason it stuck out to me was noticing that the cox version of glmnetcv needed to be updated with the rng change, and I guess in future they'll need to be kept in sync. No big deal, it's not like the package is in constant flux anyway.

I'm not sure what's going on with the broken tests now; check_jerr only has two arguments? the error claims it only has three...

I'm guessing this branch needs to sync with master again, since I went through yesterday and merged a few old PRs and fixed a number of low-hanging issues. This error probably came from https://github.com/JuliaStats/GLMNet.jl/pull/51

dsweber2 commented 3 years ago

For some reason, CategoricalArray wasn't showing up for the tests on the remote. It worked fine on my computer. So I added it as a test dependency, which seems to have solved the issue. Puzzling though, as I'm on 1.5.3 as well. I think we've addressed most of the extra stuff that came up. Thanks for keeping this maintained.

JackDunnNZ commented 3 years ago

Thanks again, looks good to me!

Marco-Congedo commented 3 years ago

Thanks guys to take the package up!