JuliaStats / GLM.jl

Generalized linear models in Julia
Other
584 stars 114 forks source link

Adding PowerLink link function in GLM #474

Closed ayushpatnaikgit closed 2 years ago

ayushpatnaikgit commented 2 years ago

PowerLink refers to the class of transforms that use a power function (e.g. a logarithm or an exponent) to transform responses into a Gaussian or a Gaussian-like.

The summaries of the changes in the following source files are:

codecov-commenter commented 2 years ago

Codecov Report

Merging #474 (634ece5) into master (429bd7c) will increase coverage by 1.81%. The diff coverage is 100.00%.

:exclamation: Current head 634ece5 differs from pull request most recent head 06d6a59. Consider uploading reports for the commit 06d6a59 to get more accurate results

@@            Coverage Diff             @@
##           master     #474      +/-   ##
==========================================
+ Coverage   85.12%   86.94%   +1.81%     
==========================================
  Files           7        7              
  Lines         827      835       +8     
==========================================
+ Hits          704      726      +22     
+ Misses        123      109      -14     
Impacted Files Coverage Δ
src/GLM.jl 50.00% <ø> (ø)
src/glmfit.jl 79.29% <100.00%> (+0.55%) :arrow_up:
src/glmtools.jl 92.30% <100.00%> (+9.60%) :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 429bd7c...06d6a59. Read the comment docs.

ayushpatnaikgit commented 2 years ago

@ViralBShah

We are testing:

 GLM.mueta(InverseLink(), 10) == GLM.mueta(PowerLink(-1), 10)

While this is passing in Julia 1.0, it is failing in Julia nightly, and we are getting the following error:

 Evaluated: -0.01 == -0.010000000000000002

I have done rounding, so this test passes.

ViralBShah commented 2 years ago

Look at some of the tests in Julia's LinearAlgebra or other numerical packages on how to handle this in tests.

mousum-github commented 2 years ago

Look at some of the tests in Julia's LinearAlgebra or other numerical packages on how to handle this in tests.

Instead of GLM.mueta(InverseLink(), 10) == GLM.mueta(PowerLink(-1), 10) (or equality checking of two real values) we should use isapprox(GLM.mueta(InverseLink(), 10), GLM.mueta(PowerLink(-1), 10)) or isapprox(GLM.mueta(InverseLink(), 10), GLM.mueta(PowerLink(-1), 10); atol=1.0e-08).

ayushpatnaikgit commented 2 years ago

We can use isapprox, but in this case, rounding seems more apt. I wanted to point out if there is a possible bug in Julia nightly that we should isolate and report.

ViralBShah commented 2 years ago

This is not indicative of a bug in Julia nightly. @mousum-github 's suggestion to use isapprox is the right one.

ViralBShah commented 2 years ago

cc @dmbates

ViralBShah commented 2 years ago

Good to merge?

ViralBShah commented 2 years ago

Bump again. @nalimilan Is this good to merge?

nalimilan commented 2 years ago

Thanks!