Multinomial and Cox redux; also fixing showarray error

[dsweber2]

This is a minor rewrite of pull request #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.

[JackDunnNZ]

Thanks for reviving this @dsweber2! I had some very minor comments related to incorporating the few PRs that have happened here in the meantime

Given that it's not your work originally, I'm not sure how much effort you are inclined to put into this, but it's a bit of a shame that the original PR had a lot of duplication between the current code and the new files, the biggest ones that stuck out to me were:

• 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
• the duplication of the glmnetcv function for cox
• duplication of the some of the generic functions like predict

I don't really have the time to go through and do this myself, but I don't want to block you if you need this added. It would be great if you were able to tidy it up, but I am happy to merge either way.

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

[codecov-io]

Codecov Report

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

@@             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%)	⬆️

---

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]

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]

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]

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 #51

[dsweber2]

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]

Thanks again, looks good to me!

[Marco-Congedo]

Thanks guys to take the package up!