search

const-ae / glmGamPoi

Fit Gamma-Poisson Generalized Linear Models Reliably
105 stars 14 forks source link

Add compute_lfc_se option to test_de #63

Closed jackkamm closed 5 months ago

jackkamm commented 5 months ago

PR as suggested by https://github.com/const-ae/glmGamPoi/issues/12#issuecomment-2132990110

const-ae commented 5 months ago

Thanks, this is great. The PR looks good to me. I haven't merged it yet, because I realized while going through the code that the current implementation of predict actually does not consider non-zero ridge penalties which leads to wrong standard errors if ridge_penalty is set to a large value. I started working on a fix (currently at https://github.com/const-ae/glmGamPoi/commit/aec87a39d5a8f9317d3f71da294ead14cac2f756), but I want to double check that my code works as expected.

Let me know what you think.

jackkamm commented 5 months ago

Thanks, yes it would be great to have the correct SE for ridge regression also.

I have to admit I'm not super familiar with computing SE for ridge GLM, so I didn't fully grok your code yet. But from a quick skim I think I found a minor typo/error that I commented on.

As an aside, I'm mainly interested in using the SEs in downstream shrinkage methods like apeglm/ashr/mashr. This makes me wonder the implications of combining the ridge shrinkage with the downstream shrinkage methods. In particular because the ridge estimates are biased and the MSE contains both a variance and square-bias component. Anyways just something to ponder on...

const-ae commented 5 months ago

I have to admit I'm not super familiar with computing SE for ridge GLM, so I didn't fully grok your code yet. But from a quick skim I think I found a minor typo/error that I commented on.

I added some extra documentation to better explain where the formulas come from (see here).

As an aside, I'm mainly interested in using the SEs in downstream shrinkage methods like apeglm/ashr/mashr. This makes me wonder the implications of combining the ridge shrinkage with the downstream shrinkage methods

I think using a ridge penalty and apeglm would be somewhat redundant. The idea of apeglm is to apply a Cauchy prior to each coefficient and make the scale data-dependant. The ridge penalty can be understood as a Normal prior on each coefficient with a fixed scale.

On the other hand, the ridge penalty can be useful to implement smoothing splines (as explained for example in this blog-post), so maybe there is some room to combine the ridge penalty with additional downstream methods.

Anyways, thank you again for implementing this feature and the well-written code :)