Posterior sample

[zouyuxin]

@gaow Could you help me to check the compute_posterior_matrices_common_cov_R function? Z is a P x J matrix. I use the for loop for j to do the sampling. The commented lines are the one using sapply. I am not sure which one is better. Also, I don't know if there is any method to avoid the for loop over j.

[gaow]

@zouyuxin thank you for the update. I think sapply should in principle at least make the code a lot clearer (if not faster), but I do not think I understood the necessity of those if ... else logic over p. Would you remind me what's special about p=1, the null?

Also, I don't know if there is any method to avoid the for loop over j.

You mean to sample from matrix normal? Did you look for existing implementations?

[zouyuxin]

@gaow The special is not about the null. I want to set those rows to the samples. So I need to find the correct index (from which row to which row). One way is using (Z_cumsum[p-1,j]+1):Z_cumsum[p,j]. When p = 1, the first Z_cumsum[1,j] rows are the samples, so I need 1:Z_cumsum[1,j] (the Z_cumsum[0,j] is numeric(0)). This is why I use the if ... else logic over p. I don't know if my explanation is clear. Do u have some suggestions about this?

When the data has equal sd, the common_cov version is more efficient than the general version, because we avoid the for loop over J. In my code, there is still a for loop over J. I don't know if there is a clearer way to avoid the loop.

[gaow]

@pcarbo Yuxin and I have simplified together her code to address to points raised in discussions above. I think I'm okay now with the general framework and core implementations. But there surely are coding practices etc that would be good to get your opinions on. Also please feel free to raise your concerns on more fundamental stuff if you spot anything wrong.

BTW I'm going to take a look at unit tests later myself.

[pcarbo]

@gaow Thanks, if it okay with you, I will wait until you & Yuxin have addressed the issues with the unit tests before I review the code.

[gaow]

@pcarbo oh okay there are a couple of things I need to catch up with mash and susie hopefully the next few days. Will sort them out and pin you here again.

[codecov-io]

Codecov Report

Merging #47 into master will increase coverage by 0.24%. The diff coverage is 72.16%.

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
+ Coverage   74.44%   74.68%   +0.24%     
==========================================
  Files          20       20              
  Lines         986     1043      +57     
==========================================
+ Hits          734      779      +45     
- Misses        252      264      +12

Impacted Files	Coverage Δ
R/data2cov.R	0% <ø> (ø)	:arrow_up:
R/simulations_contrast.R	0% <ø> (ø)	:arrow_up:
R/ed.R	0% <ø> (ø)	:arrow_up:
R/mash.R	73.25% <57.14%> (-0.12%)	:arrow_down:
R/posterior.R	89.36% <71.42%> (-1.55%)	:arrow_down:
R/posterior_common_cov.R	83.78% <72.5%> (+8.78%)	:arrow_up:
R/posterior_lowmem.R	75% <74.41%> (-6.36%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update dabd786...ffb25fb. Read the comment docs.

[gaow]

@pcarbo unit tests have been fixed. I also installed the versioning hook on my local clone so each commit now bumps internal version number, runs documentation and unit tests, before actually making a commit. We can now review & discuss this patch. I'll make some comments on the code of what I'm not sure about most (and pin you there).