Skip to content

Conversation

@ayushpatnaikgit
Copy link
Contributor

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:

  • In docs/src/api.md:
    • Added PowerLink under the “Links and methods applied to them” section
  • In docs/src/examples.md
    • Added an example “Linear regression with PowerLink. Choose the best model from a set of λs, based on minimum BIC”
  • In docs/src/index.md:
    • Added PowerLink under the “Currently the available Link types are” section
  • In src/GLM.jl:
    • Exported PowerLink
  • In src/glmfit.jl
    • Added a member variable link to GlmResp structure. This is for storing details of the link function with parameters (if any)
  • In GlmResp constructor, added one more argument to pass the link
    • Added updateμ! function specifically for PowerLink. Inside the function, the `inverselink’ function is called with PowerLink with the parameter, as the link function.
    • Added GLM.Link function specifically for PowerLink
  • In src/glmtools.jl:
    • Added PowerLink to the docstring under “GLM currently supports the following links:”
    • Defined the Powerlink structure, λ is the parameter for this link function. The user needs to specify the λ.
    • Defined linkfun function for PowerLink
    • Defined linkinv function for PowerLink
    • Defined mueta function for PowerLink
    • Defined inverselink function for PowerLink
  • In test/runtests.jl:
    • Added a set of test cases, which consisted of individual tests of all new functions related to PowerLink
    • Added a test case to compare against the R glm()
    • Added 3 test cases to check PowerLink with some special cases of PowerLink like LogLink, SqrtLink and IdentityLink.

@codecov-commenter
Copy link

codecov-commenter commented Apr 16, 2022

Codecov Report

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

❗ 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%) ⬆️
src/glmtools.jl 92.30% <100.00%> (+9.60%) ⬆️

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.

Test was failing in Julia Nightly as: 

1) GLM.linkinv(InverseLink(), 10) was 0.01 while GLM.linkinv(PowerLink(-1), 10) was 0.010000000000000002
2) GLM.linkinv(InverseSquareLink(), 10) was -10.01 while GLM.linkinv(PowerLink(-2), 10) was -0.010000000000000002

Rounding off to 2 digits should solve this. 

Note: These tests were passing in other versions of Julia.
@ayushpatnaikgit
Copy link
Contributor Author

@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
Copy link
Contributor

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

@mousum-github
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor

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

Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

ayushpatnaikgit and others added 11 commits April 18, 2022 12:34
Correcting order of PowerLink, ProbitLink, SqrtLink.

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Using ≈ instead of isapprox.

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Using alphabetical order in references.

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Writing the example description in plain text.

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Removing extra space to be consistent with the style.

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Using a better variable name for 1 by lambda.

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

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Making the doctest code concise.

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
ayushpatnaikgit and others added 3 commits April 18, 2022 13:12
Removing the R code.

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Removing test of hashes.

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
…few more metrics to test, also replaced all `=` by `≈` for real numbers
@ViralBShah
Copy link
Contributor

cc @dmbates

mousum-github and others added 4 commits April 19, 2022 17:23
Since we are storing the link - the `GLM.Link` function can be defined uniformly for all link functions

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Changing the sentence to make it compact

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
…er after d (distribution) and changed in corresponding constructor, remove updateμ! function for PowerLink and modified general updateμ! function.
Ayush Patnaik and others added 4 commits April 22, 2022 15:42
@ViralBShah
Copy link
Contributor

Good to merge?

@ViralBShah
Copy link
Contributor

Bump again. @nalimilan Is this good to merge?

mousum-github and others added 2 commits May 20, 2022 13:47
The suggestion is committed.

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@nalimilan nalimilan merged commit c96c258 into JuliaStats:master May 22, 2022
@nalimilan
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants