Adding normal prior functionality

[andrewg3311]

Hey all, Here are the files that execute the regular normal prior functionality (estimate mu and a). The functions mirror the structure of the existing "_point_normal" functions, for consistency.

[codecov-io]

Codecov Report

Merging #12 into master will decrease coverage by 11.69%. The diff coverage is 1.02%.

@@           Coverage Diff           @@
##           master   #12      +/-   ##
=======================================
- Coverage   51.69%   40%   -11.7%     
=======================================
  Files          13    18       +5     
  Lines         325   420      +95     
=======================================
  Hits          168   168              
- Misses        157   252      +95

Impacted Files	Coverage Δ
R/grad_normal.R	0% <0%> (ø)
R/compute_summary_results_normal.R	0% <0%> (ø)
R/post_sampler_normal.R	0% <0%> (ø)
R/loglik_normal.R	0% <0%> (ø)
R/mle_normal.R	0% <0%> (ø)
R/mle_point_normal.R	66.66% <33.33%> (ø)	:arrow_up:
... and 2 more

---

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 f196cbd...f4b48ba. Read the comment docs.

[willwerscheid]

@andrewg3311 A few quick comments: 1. There's a lot of cutting-and-pasting here. Could you please think about how to rewrite these functions to re-use existing code (modifying existing code where necessary)? Otherwise these functions will become very difficult to maintain. 2. There was already a function called ebnm_normal. Now there are two. The old one should be removed (but look at the old one for an example of how to re-use code). 3. Please add unit tests when adding new functionality. I will admit that the coverage is poor right now, but that's no excuse to make it worse!

[andrewg3311]

@willwerscheid 1) Can do. My first thought was to try to use the existing functions, but as they were, it didn't make sense. If I'm "allowed" to change existing functions, then I will try to re-work existing functions to be more general where needed, re-write my functions to call existing ones, etc. 2) I assumed that function would be removed, since it would be superseded by whatever functions I would write 3) Sure. It looked like there were only unit tests for the simpler functions (e.g. gradient calculations), since unit testing for the optimization is difficult. Do you have any suggestions on what I should/shouldn't add unit tests for?

[willwerscheid]

@andrewg3311 Yes, please feel free to modify existing functions. I am happy to take a look at any changes. We have not been good about writing tests for this package. I will try to add some for the point normal case over the next few days. One idea would be to simulate data from a known prior and then check that ebnm estimates something similar. But the main principle should be to test the interface (ebnm_normal, ebnm_point_normal), not so much the internal functions (calculating likelihoods, gradients, etc.).

[stephens999]

@willwerscheid andrew and i just had our weekly meeting. we agreed a two-stage strategy. First he will implement some tests (either unit tests, or some kind of benchmarking vignettes) to i) establish correctness of implementation; ii) assess reliability of results; iii) assess speed. Then we will merge the pull request, and start work on refactoring the code (eg removing unnecessary repetition, uniform interface). Refactoring the code will take some changes to existing stuff, so it seems better to put the tests in place first before moving forward on the refactoring.

[pcarbo]

Would it make most sense to create new branch in the main repository (e.g., regular-normal) for Andrew to explore these ideas? To do so, I would have to add him as a collaborator on this repo.

I agree with the approach. It makes most sense to be confident that the implementation works and does the right thing before figuring out how to best integrate with the rest of the package functionality.

[stephens999]

yes, i like that idea. Matthew

On Wed, Oct 17, 2018 at 9:54 PM Peter Carbonetto notifications@github.com wrote:

Would it make most sense to create new branch in the main repository (e.g., regular-normal) for Andrew to explore these ideas? To do so, I would have to add him as a collaborator on this repo.

I agree with the approach. It makes most sense to be confident that the implementation works and does the right thing before figuring out how to best integrate with the rest of the package functionality.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/stephenslab/ebnm/pull/12#issuecomment-430857964, or mute the thread https://github.com/notifications/unsubscribe-auth/ABt4xV4rzbOibw0L-M0sLhn7Tw0b8uZhks5ul-1fgaJpZM4XkXOk .

[pcarbo]

@andrewg3311 I've added you as a collaborator to this repo. If you are not sure how to create a new branch, I can do it for you, or explain how.

[andrewg3311]

@pcarbo I believe I just added the branch. Please check to make sure I did it right. Thanks!

[pcarbo]

@andrewg3311 Try committing a change to a file in that branch and pushing that change.

[andrewg3311]

@pcarbo Just tested it, looks to me like it's working (as is the Slack plugin you set up)

[pcarbo]

@andrewg3311 Looks good!