wwiecek / baggr

R package for Bayesian meta-analysis models, using Stan
GNU General Public License v3.0
48 stars 12 forks source link

Updating loo_compare documentation and interpretation #160

Closed dannytoomey closed 1 year ago

dannytoomey commented 1 year ago

Hi Witold,

Here's a commit that resolves issue #152. I thought the loocv() documentation made sense (enough to educate me going in with minimal subject knowledge) and I added a bit to the loo_compare documentation.

I also added an interpretation for the comparisons, which I hope isn't too over simplified, and fixed the negative SE when printing an loocv object.

Best regards,

Danny Toomey

wwiecek commented 1 year ago

@dannytoomey let me know if this is ready to review. I see you adding commits but don't know if it's time to review. I think there is a proper GH way of doing a review where you can let me know that comments have been resolved: can you check? May be a good way for us both to learn this method

dannytoomey commented 1 year ago

@wwiecek ready for review! I resolved the comments threads where all changes have been addressed. I left open the UseMethod thread in case you would like to add the ability to pass a list arguments like loo_compare(list(loo,loo2,loo3)). If not, then all changes have been addressed.

In terms of the proper way to notify you that changes are ready for re-review, I see a button I can press for that so I'll try that after posting this comment and see how that works on your end.

wwiecek commented 1 year ago

Yeah, that works well! I will test and merge if it passes checks

You're getting useMethod warning because the thing is written for objects of class baggr. The solution would have been to write a branching logic depending on whether the argument is baggr or list. Just FYI.

dannytoomey commented 1 year ago

Removed .so files and updated .gitignore