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

Add plot_bf #116

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

PiyushPanwarFST
Copy link

@PiyushPanwarFST PiyushPanwarFST commented Jan 25, 2025

Add function to plot Bayes Factor approximated as the Savage-Dickey density ratio algorithm

Bayes Factor Plotted graph

ADD figure once you implement the function in arviz_plots, instead of importing it from ArviZ

@PiyushPanwarFST
Copy link
Author

PiyushPanwarFST commented Jan 25, 2025

Sir I had done some changes in multiple files for preventing the ruff errors except init file because they are causing conflicts and these changes do not effect the functionality of the whole codebase.

Copy link
Contributor

@aloctavodia aloctavodia left a comment

Choose a reason for hiding this comment

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

Please, avoid mixing two separate issues in a single PR. Ideally, you should do one to add Bayes_factor and another for fixing ruff or whatever.

@@ -0,0 +1,120 @@
import logging
from arviz.plots.plot_utils import get_plotting_function
Copy link
Contributor

Choose a reason for hiding this comment

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

You can not use ArviZ. We final goal of the refactoring is that the ArviZ package you are importing will not exist anymore.

The rest of the implementation is not how plots are implemented in Arviz-plots. Please familiarize yourself with the library before adding more changes. See how other plots are implemented, like plot_energy or plot_psense_dist

Comment on lines 83 to 89
try:
bf, p_at_ref_val = bayes_factor.bayes_factor(
idata, var_name, prior=prior, ref_val=ref_val, return_ref_vals=True
)
except AttributeError as e:
_log.error("Error: %s", e)
raise AttributeError("Bayes factor function not found in arviz.stats.")
Copy link
Contributor

@aloctavodia aloctavodia Jan 25, 2025

Choose a reason for hiding this comment

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

No need to do this, the function should be available if all packages are properly installed

@aloctavodia aloctavodia changed the title Add plot_bf function for plotting functionality of bayes_factor #144 Add plot_bf Jan 25, 2025
@aloctavodia
Copy link
Contributor

Also, try to avoid writing very long messages, a lot of text makes it difficult to parse what is useful and what is just blah, blah, blah. Adding information is ok, but try to add only the most relevant information. Adding visuals is a good idea as this is a plotting library. If needed you can add more than one plot, to showcase the effect of different arguments.

@PiyushPanwarFST
Copy link
Author

Sir, is it okay that I have added the get_plotting_function directly to the utils.py file? Because this function is not available in arviz-base or arviz-stats, so I think we should initialize this function from scratch in arviz-plot so that we can import it and use it in plot_bf.

@aloctavodia
Copy link
Contributor

No, you don't need that function. Only use functions from arviz-base, arviz-stats and arviz-plots. The approaches in the old arviz and arviz-plots are completely different, don't try to follow the code in the old one. Instead, think in terms of the elements you need to add. Essentially the minimum elements to get a bayes_factor plot are two distributions and a reference line. Check similar plots like plot_energy and plot_psense_dist, which will provide a guide of what you need to implement. But before that read the documentation https://arviz-plots.readthedocs.io/en/latest/index.html

@aloctavodia
Copy link
Contributor

@PiyushPanwarFST are you still working on this?

@PiyushPanwarFST
Copy link
Author

PiyushPanwarFST commented Feb 4, 2025

Sir yes I am working on it and its about to be completed, but I stuck somewhere in dataarray or dataset in bayes_factor

@aloctavodia
Copy link
Contributor

ok, just checking.

@PiyushPanwarFST
Copy link
Author

@aloctavodia I created a new file called bfplot in ArviZ-Plots to add the plotting functionality for Bayes Factor. While calling the bayes_factor function from arviz-stats for computation, I needed to extract the posterior and prior groups, but the prior group is not available in ArviZ-Plots.

Even though I loaded the data by adding a new file in inference_diagnostics using load_arviz_data("centered_eight"), the only groups I can see are:
/posterior, /log_prior, /log_likelihood, /sample_stats, and /observed_data. While trying to solve this issue, I found that in test_plots, generate_base_data only contains these groups.

Can you please guide me how to solve this issue ?

@aloctavodia
Copy link
Contributor

Try to write short comments, this could have been "The centered_eight InferenceData does not have a prior group, where I can find an Inference Data with one".

Not sure what you are doing but "centered_eight" has many groups, including a prior group. This is what I get when I do azb.load_arviz_data("centered_eight").groups

('/',
 '/posterior',
 '/posterior_predictive',
 '/log_likelihood',
 '/sample_stats',
 '/prior',
 '/prior_predictive',
 '/observed_data',
 '/constant_data')

@PiyushPanwarFST
Copy link
Author

PiyushPanwarFST commented Feb 22, 2025

@aloctavodia I have managed to plot the Bayes Factor (BF) for the prior and posterior , but a few tasks are still pending. I wanted to share my progress and get your feedback.

Screenshot 2025-02-22 at 10 27 18 AM

Still figuring out:

  1. How to add a legend for prior/posterior at the top-right.
    I checked ⁠ energyplot.py:134 ⁠ (link), where they convert ⁠ energy ⁠ into ⁠ dim ⁠ at L143. Need to understand how to do that here.

  2. In our graph, how can we add an intersecting reference line at ref_val = 0 for the prior and posterior distributions?

3.⁠ ⁠Noob question: How can I assign different colors for prior and posterior?

@aloctavodia
Copy link
Contributor

Please add a commit and discuss concrete code

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.

2 participants