Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added delta method for glm links #37

Merged
merged 15 commits into from
Mar 24, 2022

Conversation

spinkney
Copy link
Contributor

I added the delta method for SE's after transformation. This pr adds an input into the effects function for the inverse link. The derivative of the effect is calculated using ForwardDiff and the delta method is used to construct the SEs.

I typically do not code in Julia so this may not be very Julian. It only took a few lines of code that I needed for a project I was working on. As such, you may use this pr as a reference to make the code more suitable for your needs and delete this pr.

@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #37 (fb38da4) into main (b8dfe4c) will not change coverage.
The diff coverage is 100.00%.

❗ Current head fb38da4 differs from pull request most recent head 1d72d16. Consider uploading reports for the commit 1d72d16 to get more accurate results

@@            Coverage Diff            @@
##              main       #37   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           83        86    +3     
=========================================
+ Hits            83        86    +3     
Impacted Files Coverage Δ
src/regressionmodel.jl 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

src/regressionmodel.jl Outdated Show resolved Hide resolved
src/regressionmodel.jl Outdated Show resolved Hide resolved
src/regressionmodel.jl Outdated Show resolved Hide resolved
src/regressionmodel.jl Outdated Show resolved Hide resolved
@ararslan
Copy link
Member

Thanks for the contribution, @spinkney! Amusingly, my comments all pertain to @palday's additions to your PR. 😄

@palday
Copy link
Member

palday commented Mar 15, 2022

@spinkney thanks for your contribution! I wanted to just add tests and expand documentation, but the original math gave incorrect results. In the simplest identity case, the derivative is just a bunch a bunch of ones regardless of X, so the error is invariant to input, which isn't correct. I've fixed that here, but it might be relevant to your other project.

@spinkney
Copy link
Contributor Author

Nice, I didn't use the identity. Is it still wrong for other links?

@palday
Copy link
Member

palday commented Mar 15, 2022

@spinkney I think there's another multiplication by the original X missing in your original formulation for other links, but I'm not sure anymore. I've checked the new approach I did against emmeans and effects in R and against the predict method in GLM.jl. We wanted to make the code here as general as possible, but realistically, we could have written specialized methods for GLM.jl that computed the reference grid and then called predict instead of computing the effects ourselves. (Downside is more code and another dependency, which is why didn't go that route.)

@palday palday requested a review from ararslan March 15, 2022 18:01
Copy link
Member

@kleinschmidt kleinschmidt left a comment

Choose a reason for hiding this comment

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

Looks good to me. This is a new feature but shouldn't be breaking (I'm assuming that the existing tests already cover the actual numbers that effects returns) so bump patch and I think it's good to go.

Comment on lines +28 to +33
By default (`invlink=identity`), effects are computed on the scale of the
transformed response. For models with an explicit transformation, that
transformation is the scale of the effects. For models with a link function,
the scale of the effects is the _link_ scale, i.e. after application of the
link function. For example, effects for logitistic regression models are on
the logit and not the probability scale.
Copy link
Member

Choose a reason for hiding this comment

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

why is this the default when the link can (in principle) be obtained from the model itself? to avoid a dependency on GLM?

Copy link
Member

Choose a reason for hiding this comment

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

Exactly, to avoid the dependency on GLM. As part of the associated docs issue (#38), I think it would be good to have a GLM example. If we decide to take GLM as a dependency, then we should add more specialized methods that take advantage of GLM.mueta rather than using AD for the derivative.

src/regressionmodel.jl Show resolved Hide resolved
src/regressionmodel.jl Outdated Show resolved Hide resolved
@palday palday merged commit 46e5662 into beacon-biosignals:main Mar 24, 2022
@ararslan
Copy link
Member

Apologies for missing the re-review request for this but thanks @kleinschmidt for reviewing, @palday for getting it over the finish line, and of course @spinkney for the contribution. 🙂

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.

4 participants