Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compare --> loo_compare #93

Merged
merged 19 commits into from
Mar 6, 2019
Merged

compare --> loo_compare #93

merged 19 commits into from
Mar 6, 2019

Conversation

jgabry
Copy link
Member

@jgabry jgabry commented Dec 20, 2018

This PR deprecates the compare() function in favor of loo_compare(). This is for several reasons:

  • compare() is not a generic with methods, which results in rstanarm, brms, etc., using different names for wrappers around compare(). With loo_compare() we can have methods loo_compare.stanreg, loo_compare.brmsfit, etc, unifying the names.

  • compare() also conflicts with function names in other packages (e.g. testthat::compare)

We should time the release of this so that the loo_compare() methods in brms and rstanarm are released at the same time.

@paul-buerkner
Copy link
Contributor

Hey Jonah! I will look at the code tomorrow and test it together with the loo_compare stuff of brms to make sure everything is compatible.

Copy link
Contributor

@paul-buerkner paul-buerkner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me and works smoothly with the corresponding changes I made in brms.
I think this is ready to be merged.

@paul-buerkner
Copy link
Contributor

One little thing: Could we add an is.kfold function as well?

@jgabry
Copy link
Member Author

jgabry commented Jan 3, 2019

I'm ready to merge this but @avehtari do you want to first try out the loo_compare() function (on the loo_compare) branch and let us know if you like the interface/output?

@codecov-io
Copy link

codecov-io commented Jan 5, 2019

Codecov Report

Merging #93 into master will decrease coverage by 1.07%.
The diff coverage is 86.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
- Coverage   94.31%   93.24%   -1.08%     
==========================================
  Files          15       17       +2     
  Lines        1197     1259      +62     
==========================================
+ Hits         1129     1174      +45     
- Misses         68       85      +17
Impacted Files Coverage Δ
R/kfold-helpers.R 100% <ø> (ø) ⬆️
R/helpers.R 98.52% <ø> (-0.09%) ⬇️
R/print.R 92.45% <0%> (-5.55%) ⬇️
R/kfold-generic.R 0% <0%> (ø)
R/psis.R 90.66% <100%> (+0.06%) ⬆️
R/loo.R 92.61% <100%> (+0.07%) ⬆️
R/compare.R 83.63% <100%> (-11.75%) ⬇️
R/waic.R 100% <100%> (ø) ⬆️
R/loo_compare.R 91.04% <91.04%> (ø)
... 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 db9fd3f...190f79a. Read the comment docs.

@jgabry
Copy link
Member Author

jgabry commented Feb 27, 2019

@paul-buerkner I removed the deprecation warning from compare() for now as we discussed. I'm running into a few issues with reverse dependency checks and I need to sort out if they are loo related, but otherwise I think this is ready to go.

@jgabry jgabry merged commit 41f899a into master Mar 6, 2019
@jgabry jgabry deleted the loo_compare branch March 12, 2019 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants