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

loo.function should not require a r_eff argument #106

Open
bgoodri opened this issue Apr 16, 2019 · 7 comments
Open

loo.function should not require a r_eff argument #106

bgoodri opened this issue Apr 16, 2019 · 7 comments

Comments

@bgoodri
Copy link

bgoodri commented Apr 16, 2019

If the user is providing a function to calculate the log-likelihood contribution of each observation, why does the user also need to pass a call to relative_eff? This just seems unnecessary.

@jgabry
Copy link
Member

jgabry commented Apr 16, 2019

It does seem unnecessary. We can drop that and calculate it internally.

@jgabry
Copy link
Member

jgabry commented Apr 16, 2019

Actually, computing r_eff requires knowing which chain each posterior draw comes from. The chain id is not a required input to loo.function because LOO just needs all draws stacked together. In other words, dropping the r_eff argument would require adding a chain_id argument to loo.function or requiring a chain_id column in draws so that r_eff can be calculated internally.

Here are three possibilities:

# drop the r_eff argument and require a chain_id argument
loo.function(x, ..., data, draws, chain_id)

# drop the r_eff argument and require chain_id to be included in draws
loo.function(x, ..., data, draws) # checks internally for a chain_id column in 'draws'

# keep the r_eff argument and add a chain_id argument so the user can 
# decide whether to compute r_eff and pass it in or provide chain_id and
# let loo calculate r_eff
loo.function(x, ..., data, draws, r_eff, chain_id)

Anyone have a preference or other suggestions?

@bgoodri
Copy link
Author

bgoodri commented Apr 16, 2019 via email

@jgabry
Copy link
Member

jgabry commented Apr 16, 2019

Yeah if draws is an array then we don’t need the chain_id. I don’t think we should require it to be an array, but we could go with option (1) above but only requiring chain_id if draws is not an array.

@avehtari and @paul-buerkner any preference here?

@avehtari
Copy link
Collaborator

Now that we have posterior package, we have chain and iteration info often, and could compute r_eff based on those, and maybe then for the arrays/matrices without that info assume no r_eff computation unless additional information is provided?

@avehtari
Copy link
Collaborator

Bumping this as I'm now in favor of reducing the need to enter r_eff. I would be fine with that r_eff=1 unless we do get chain and iteration information via draws object, and in those cases estimate r_eff internally

@avehtari
Copy link
Collaborator

#235 reduced the need to provide r_eff. @bgoodri does that solve your problem?

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

No branches or pull requests

3 participants