Skip to content

Conversation

@aloctavodia
Copy link
Member

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

other than the extra files with .md instead of .myst.md extension it looks good

all += perct_per_class
print(perct_per_class)
all
az.plot_ppc_pava(idata_t, data_type="categorical");
Copy link
Member

Choose a reason for hiding this comment

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

My main comment is probably not actionable but might be worth to keep in mind for arviz development. I am not sure I can see this as it is a comparison between this figure and one a few sections higher. Does plot_ppc_pava support dict input to show both models, if not, do you think that is useful to add?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the only ones supporting more than one model are plot_forest and plot_dist. Not sure how useful, it could be ok for plot_ppc_pava and maybe plot_ppc_pit. Maybe something more general is to have something similar to combine_plots but that works for different data. So we can create a single figure with one model per row or column, so comparisons are easier to see and present.

Copy link
Member

Choose a reason for hiding this comment

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

this one also seems to be an extra file. There might be something broken in the jupytext settings :/

)
ax[1].set(title="Posterior Predictive Check (test)", xlim=(0, 1_000));
```{code-cell} ipython3
az.plot_ppc_dist(posterior_predictive_oos_regression_test, kind="ecdf");
Copy link
Member

Choose a reason for hiding this comment

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

might be more distracting than helpful now that the plot is a single line, but we could keep the 0-1000 xlims with

Suggested change
az.plot_ppc_dist(posterior_predictive_oos_regression_test, kind="ecdf");
pc = az.plot_ppc_dist(posterior_predictive_oos_regression_test, kind="ecdf")
pc.get_viz("plot", "y").set_xlim(0, 1_000);

Copy link
Member Author

Choose a reason for hiding this comment

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

could be, but also important to show the syntax.

@fonnesbeck
Copy link
Member

arviz.preview? Do we want to wait on this until its no longer in preview? We will just have to go back and change this again, right?

@aloctavodia
Copy link
Member Author

aloctavodia commented Dec 3, 2025

I think we should not wait. Once we have ArviZ 1.0, we will need to update one import. I will do the update.

Edit:
@fonnesbeck, not sure my previous answer was that useful. My main motivation with this PR is to check that "new arviz" has all the functions that we want and they work as expected, and that others can provide feedback. I think is Ok to merge this eventually, but fine with me if we want to wait for ArviZ 1.0 (or RC).

@OriolAbril
Copy link
Member

OriolAbril commented Dec 3, 2025

I think it is probably better to start the migration before the actual 1.0 release. After that we'll only need to replace arviz.preview by arviz. I think it will also help signal users they should start preparing for the migration which will make the update smoother.

Moreover, if we wait, we won't be able to update any page until pymc has fully migrated internally. That means there are many new features like dedicated ppc plots, prior sensitivity checks, loo with moment matching or subsampling, advanced facetting and aesthetic mappings that are ready to use but can't be used because we don't want to use arviz.preview.

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.

3 participants