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 autocorrelation plot #153

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

Conversation

suhaani-agarwal
Copy link

@suhaani-agarwal suhaani-agarwal commented Feb 25, 2025

This is in reference to Issue #2
Current State:

  • This is the initial commit for autocorrelation plot.
  • The code is partially functional but still has some issues.
  • The acf_data is being computed correctly from autocorr function from arviz-stats, but there are warnings about missing selections in viz[autocorr]-
    Warning: Selection (('chain', np.int64(0)), ('draw', np.int64(0)), ('lag', np.int64(0))) not found in viz[autocorr].
    This indicates a mismatch in the expected dimensions or coordinates in the viz dataset.
  • The plot is not yet fully functional due to this issue.

I would greatly appreciate your review and guidance on any changes needed to address errors or improve the implementation.


📚 Documentation preview 📚: https://arviz-plots--153.org.readthedocs.build/en/153/

# def get_target(self, var_name, selection):
# """Get the target that corresponds to the given variable and selection."""
# return subset_ds(self.get_viz(var_name), "plot", selection)

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't change this, focus on the plot

plot_collection : PlotCollection
The plot collection containing the autocorrelation plots.
"""
dt = convert_to_dataset(dt, group="posterior")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dt = convert_to_dataset(dt, group="posterior")


# Calculate autocorrelation from arviz_stats autocorr computation
core_base = _CoreBase()
acf_data = core_base.autocorr(distribution_array, axis=-1)[..., :max_lag]
Copy link
Contributor

Choose a reason for hiding this comment

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

If autocorr is not an accessor it should be, then you will be able to pass ds/da.

Copy link
Author

Choose a reason for hiding this comment

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

sir, you mentioned making autocorr work with xarray objects. Should I :
-Raise an issue in arviz_stats to modify autocorr for xarray support,
or
-Create a wrapper/accessor in autocorrplot.py locally?

n_plots, plots_per_var = process_facet_dims(pc_data, pc_kwargs["cols"])

# Set up figure size
figsize = pc_kwargs.get("plot_grid_kws", {}).get("figsize", None)
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 copy all the "if plot_collection is None" logic from other plots like plotdist

@suhaani-agarwal
Copy link
Author

@aloctavodia sir, you mentioned making autocorr work with xarray objects. Should I :
-Raise an issue in arviz_stats to modify autocorr for xarray support,
or
-Create a wrapper/accessor in autocorrplot.py locally?

@aloctavodia
Copy link
Contributor

Arviz-stats already have accessors for a few functions like hdi, ess, etc. So that's the place to implement them.

@aloctavodia
Copy link
Contributor

@suhaani-agarwal are you still working on this?

@suhaani-agarwal
Copy link
Author

@suhaani-agarwal are you still working on this?

yes, I am still working on this. I am stuck in some dimension related properties of the autocorr computation.

@aloctavodia aloctavodia changed the title Initial work on autocorrelation plot refactor Add autocorrelation plot Mar 9, 2025
@suhaani-agarwal
Copy link
Author

suhaani-agarwal commented Mar 12, 2025

Hello , @aloctavodia this is some progress on the code....I have added the computation using the autocorr accessor, still a lot of work is remaining. I have some doubts...

  1. Is it okay to use the for loop for computation of data for every variable? if not, what else can I do? because when I am not using the loop, I am getting certain errors on the handeling of dimensions.
  2. right now, encountering KeyError: 'autocorr' due to incorrect dataset structure—tried multiple fixes but issue persists. Can you suggest a way to resolve the same?

@aloctavodia
Copy link
Contributor

It is incorrect to do the looping manually. See for example https://github.com/arviz-devs/arviz-plots/blob/main/src/arviz_plots/plots/ecdfplot.py,

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