Correct bfmi denominator (fixes #858) #991
Conversation
|
can someone tell me why this fails? I think the desired value in test_bfmi() should be changed according to the new formula? |
|
Hi @nitishp25 , I think you have to change the test_bfmi() function of test_diagnostics.py. Run the code in test function with updated ddof value and replace the result in assert_almost_equal() function. |
|
Got it, thanks @percygautam! |
|
Ready to merge. |
arviz/stats/diagnostics.py
Outdated
| @@ -526,7 +526,7 @@ def _bfmi(energy): | |||
| energy_mat = np.atleast_2d(energy) | |||
| num = np.square(np.diff(energy_mat, axis=1)).mean(axis=1) # pylint: disable=no-member | |||
| if energy_mat.ndim == 2: | |||
| den = _numba_var(svar, np.var, energy_mat, axis=1, ddof=0) | |||
| den = _numba_var(svar, np.var, energy_mat, axis=1, ddof=1) | |||
| else: | |||
| den = np.var(energy, axis=1) | |||
There was a problem hiding this comment.
I think that numpy's default is ddof=0, so this line in the else should be corrected too. Either way, even if the default were ddof=1 it's probably better to make it explicit in this case.
There was a problem hiding this comment.
Yup, the default is 0. I'll change this. Thanks for the feedback @OriolAbril!
|
Also, this is a little unrelated but it would be great if you could fix it. I have just seen that bfmi docs are not rendered properly (see rhat docs to see how they should look like) there is no |
Got it. Working on it. |
|
This looks like the right fix. With the fix to the else statement suggested by @OriolAbril, this looks good to me. Thanks for the PR! |
|
Please review @OriolAbril |
|
Thanks for you PR! |
Correct bfmi denominator (arviz-devs#991)
* correct bfmi denominator * update test_diagnostics.py * fixed denominator and docs
* integrated the point_estimate param with rcparam * added black formatting * added black formatting * created new function calculate_point_estimate * changed the location of _fast_kde function from kdeplot.py to plot_utils.py * Correct bfmi denominator (#991) * correct bfmi denominator * update test_diagnostics.py * fixed denominator and docs * changes * dependency issue solved * linting changes * linting changes * changes * final changes * linting correction * moved _fast_kde_2d function to plot_utils.py * minor linting changes Co-authored-by: Nitish Pasricha <pasrichanitish2@gmail.com>
Correction for bfmi formula implementation according to page 7 of this paper.
Fixes #858.
Fix denominator in if statement
Fix denominator in else statement
Fix bfmi docs to render example properly