Closed spinkney closed 2 years ago
Merging #37 (fb38da4) into main (b8dfe4c) will not change coverage. The diff coverage is
100.00%
.:exclamation: 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%> (ø) |
:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more
Thanks for the contribution, @spinkney! Amusingly, my comments all pertain to @palday's additions to your PR. :smile:
@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.
Nice, I didn't use the identity. Is it still wrong for other links?
@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.)
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. :slightly_smiling_face:
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 usingForwardDiff
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.