JuliaStats / GLM.jl

Generalized linear models in Julia

Other

584 stars 114 forks source link

support heterogeneous array types #466

Closed piever closed 2 years ago

piever commented 2 years ago

Fix #213

This simply performs a conversion step before building GlmResp or LmResp to make sure that all arrays have the same type before proceeding to construct the object.

~It still does not allow to pass an abstract matrix as the first argument to GLM.glm, because of this signature, but I imagine that can be addressed separately.~

EDIT: the second commit also adds support for abstract matrices.

kleinschmidt commented 2 years ago

Looks like you also made the change to support AbstractMatrix in glm?

codecov-commenter commented 2 years ago

Codecov Report

Merging #466 (0d3e3a9) into master (be96833) will increase coverage by 0.27%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #466      +/-   ##
==========================================
+ Coverage   84.01%   84.29%   +0.27%     
==========================================
  Files           7        7              
  Lines         807      815       +8     
==========================================
+ Hits          678      687       +9     
+ Misses        129      128       -1

Impacted Files	Coverage Δ
src/glmfit.jl	`79.02% <100.00%> (+0.65%)`	:arrow_up:
src/lm.jl	`96.85% <100.00%> (+0.10%)`	: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 be96833...0d3e3a9. Read the comment docs.

piever commented 2 years ago

Thanks for the quick review!

Looks like you also made the change to support AbstractMatrix in glm?

Yes, in the end it was a very small change, so it felt cleaner to include it here.

I've bumped the patch number, it would actually be great to tag a release after merging this (so I can remove the workarounds in AlgebraOfGraphics).

kleinschmidt commented 2 years ago

I think it's worth looking at how this interacts with #443 and #446 so I've requested review from @andreasnoack as well.

andreasnoack commented 2 years ago

So https://github.com/JuliaStats/GLM.jl/pull/446 is slightly more general since it also covers a fix for sparse X which I don't think is covered by the changes here so I'll suggest that we go with that PR. I've taken the extra tests from this PR since they were more extensive than what had written.

piever commented 2 years ago

Sure! Closing this as it was superseded by #446. Glad the tests were useful!