creates a generic elpd function#159
Conversation
bnicenboim
changed the title
jgabry
left a comment
jgabry
left a comment
There was a problem hiding this comment.
This looks great, thanks Bruno! I made just a few tiny review comments.
@paul-buerkner If you have a chance can you also take a quick look at this in case you have any other suggestions or noticed a potential issue I missed. It's a small PR so should just take a few minutes.
Co-authored-by: Jonah Gabry <jgabry@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #159 +/- ##
==========================================
- Coverage 95.30% 95.15% -0.16%
==========================================
Files 27 28 +1
Lines 2642 2662 +20
==========================================
+ Hits 2518 2533 +15
- Misses 124 129 +5
Continue to review full report at Codecov.
|
|
I wasn't thinking about this before when reviewing, but we should also add a few tests for this. @bnicenboim are you familiar with using the testthat framework? |
|
yeah, but test it against what? As in test_loo_and_waic.R against a reference? |
|
You could test that the matrix and array methods give the same answer when they should (although that's less necessary now that the array method calls the matrix method, but I guess it's still good to have a test that will let us know if we mess that up in the future). And yeah you could also compare to a reference value like we do for the others. That way we can make sure that the results don't change accidentally. So there's not too much to test here (it's a pretty simple function) but it's still good to have a few basic sanity checks. |
|
ok, done, I just added them to the loo_waic test file since they were so similar |
|
in the meanwhile I also did a vignette, I hope it's fine that I'm adding this to the same PR |
Awesome, thanks!
Yeah, that's great. I haven't had a chance to go through the vignette yet but I'll take a look soon. |
|
I read through the vignette and I think it's great. I also pushed a commit to the PR branch adding you to the DESCRIPTION file as a contributor and adding an item to NEWS. This seems ready to merge so I'm going to approve this. Will wait a little while to merge just in case @avehtari or @paul-buerkner wants to take a look before we merge this. |
|
Looks good to me. Thank you @bnicenboim! |
avehtari
left a comment
avehtari
left a comment
There was a problem hiding this comment.
This is a great addition. I went through the vignette and have some requests for changes.
|
Thanks Aki! |
|
All ok |
|
Thanks for all the suggestions Aki and thanks for making the changes so quickly Bruno. Will merge this now! |
Actually it looks like maybe I never pushed this. I'll add you as a contributor and add a news item now on the master branch. |
|
cool, and BTW, when does the vignette get compiled? When the new version goes to CRAN? |
|
Yeah when it goes to CRAN. We don't really have to wait until then, we could add it to the website now, but if we did that then people wouldn't be able to run the code with the CRAN version of loo (because it doesn't have the elpd function yet). But we can do a CRAN release soon. In fact I'll open an issue now to see if anyone else has anything they were hoping to get in to the next release. If not then I can start preparing the release. |
5 participants
No description provided.