rss icon indicating copy to clipboard operation
rss copied to clipboard

Merge "examples-peter" with "master"

Open pcarbo opened this issue 9 years ago • 3 comments

Hi Xiang,

Can you please review & accept my pull request, making modifications as you see fit?

I successfully completed a run of example1.m on the PPS cluster. Here is a summary of the results I obtained:

model  mean(pvesam)
bvsr      0.2051
bslmm     0.2130
ash       0.1858

Main changes:

  • Replaced function pctfile with percentile.
  • Replaced function unidrnd with randint.
  • Added a script parameter to example1.m to specify where the data and results are stored.

Let me know if you have any questions or need any assistance with this pull request.

Thanks, Peter

pcarbo avatar Aug 18 '16 18:08 pcarbo

I also added a startup.m file which I use to load the required MATLAB functions (e.g., lightspeed), but feel free to remove this.

pcarbo avatar Aug 18 '16 18:08 pcarbo

Hi Peter,

Thanks! I have a few comments on the changes. Please let me know whether they looks good to you, and then I will merge this pull request immediately.

  1. place percentile.m under the misc folder:
    I think this function might be useful in general, and it is not directly used by any main subroutines (rss_bvsr.m, rss_bslmm.m and rss_ash.m), so misc seems to be a better place for this function.
  2. hide randint.m inside propose_gamma.m and update_zlabel.m:
    Although this is an obvious duplicate, I am incline to add randint.m as a local function of both propose_gamma.m and update_zlabel.m. My main reason is that randint.m is only called by these two files.

Once I merge this pull request, I will add two unit tests:

  • [ ] test percentile.m by prctile.m in stat toolbox
  • [ ] test randint.m by unidrnd.m in stat toolbox

xiangzhu avatar Aug 18 '16 19:08 xiangzhu

Xiang, that all sounds great to me.

Also, I forgot to mention this before, but please do not delete the examples-peter branch after the pull request. I want to keep this branch to work through example2.m. Thanks.

pcarbo avatar Aug 18 '16 20:08 pcarbo