search

stephenslab / rssr

Regression with summary statistics (RSS) package for R.
10 stars 2 forks source link

Recent audit of my original matlab codes #6

Closed xiangzhu closed 7 years ago

xiangzhu commented 7 years ago

Hi @CreRecombinase,

Recently I have been performing (another round of) code audit of my original Matlab implementation of rss_varbvsr. No major issues were identified. However, there are two minor issues.

  1. User-specified values of alpha are accidentally modified.

This issue corresponds to commit 0b4cd12. There was a duplicate line of code that accidentally changed the value of alpha provided by users. This duplicated line might be problematic if users provide a very good starting value of alpha (e.g. ground truth). In contrast, it is much less problematic when users provide a random starting value, which is very common in practice.

  1. The variational lower bound at initial values are not computed.

This issue corresponds to commits d9bcdca, d53572b and c637eaf. In previous versions, I did not compute the variational lower bound lnZ at initial values and simply assigned -Inf to it. The intuition is that lnZ at random initial values tend to be very small, especially in high-dimensional case. Similar to the issue above, this simplification does not affect the results if the initial values of [alpha, mu] are randomly assigned. However, this might cause problem if the initial values are well chosen (e.g. ground truth).

To address these two minor issues, I create this pull request. Please review and test it at your earliest convenience. Please let me know if there is any problem.

The main algorithm is independent of these two issues, so I speculate that neither of these issues would affect your current implementation and testing results, but I think it is important for us to fix them at an early stage of package development.

CreRecombinase commented 7 years ago

These changes look good to me. I'll try to add some tests that cover these cases.