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 summary function #51

Merged
merged 11 commits into from
Feb 4, 2025
Merged

Add summary function #51

merged 11 commits into from
Feb 4, 2025

Conversation

aloctavodia
Copy link
Contributor

@aloctavodia aloctavodia commented Jan 27, 2025

Closes #11

This adds the basic functionality for the summary function. It is still not on par with the summary function from old ArviZ, but it is better than nothing. Currently, the output is a dataset. To get a Pandas' DataFrame we need arviz-devs/arviz-base#25

In the old arviz we had a kind argument and a stat_focus argument. The combination of both specifies which stats/diagnostics were computed. Now we achieve the same with just the kind argument. For instance kind="all" is the equivalent of the old kind=all and stats_focus="mean" and kind kind="all_median" is the equivalent of the old kind=all and stats_focus="median". Having a single argument seems clear to me.

One source of inconsistency is that previously we returned the hdi if stat_focus="mean" and the eti if stat_focus="median . But in arviz 1.0 whether to compute these two different CI depend on a global rcparam value. Should we ignore the rcparam specification and return what the kind argument dictates? If "stats_median", or "all_median" we return eti irrespective of the value of ci_kind (or the value in rcparams).


📚 Documentation preview 📚: https://arviz-stats--51.org.readthedocs.build/en/51/

@amaloney
Copy link
Member

tag me when you are ready for a review

@aloctavodia aloctavodia requested a review from amaloney January 28, 2025 06:15
Copy link
Member

@amaloney amaloney left a comment

Choose a reason for hiding this comment

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

This looks great. Just a few nits about the docstring and type-hints, if you are okay with the suggested changes.

The only thing I would request an actual change for is the default value in the docstring does not match that in the function signature.

aloctavodia and others added 2 commits February 2, 2025 11:03
Co-authored-by: Andy Maloney <amaloney@mailbox.org>
@aloctavodia
Copy link
Contributor Author

I fixed the docstring and added type hints.

Please check if they are ok, feel free to make changes if you want. Not sure about the type hints for data, currently the function accepts DataSet, DataTree and InferenceData. Support for InferenceData should be transitional as in ArviZ 1.0 we replace it with DataTree.

Copy link
Member

@amaloney amaloney left a comment

Choose a reason for hiding this comment

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

I think this is a great start. There are some new plumbings in Python 3.13 that will make more complicated types easier to handle, but I think what you have now will work just fine.

@amaloney amaloney merged commit fc94340 into main Feb 4, 2025
3 checks passed
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.

Add summary
2 participants