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.

Sign up for GitHub

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

Jump to bottom

Stats docs [GSoC] #670

Merged
merged 11 commits into from
May 23, 2019
Merged

Stats docs [GSoC] #670

merged 11 commits into from
May 23, 2019

Conversation

OriolAbril
Copy link
Member

@OriolAbril OriolAbril commented May 15, 2019 •
edited
Loading

Minor fixes on stats module docs and adding examples. Fixes #631

@canyon289
Copy link
Member

LGTM

@ahartikainen
Copy link
Contributor

These are annoying

************* Module arviz.stats.diagnostics
arviz/stats/diagnostics.py:120:0: C0301: Line too long (102/100) (line-too-long)
arviz/stats/diagnostics.py:182:0: C0301: Line too long (102/100) (line-too-long)
************* Module arviz.stats.stats
arviz/stats/stats.py:382:0: C0301: Line too long (106/100) (line-too-long)

@OriolAbril
Copy link
Member Author

I will probably finish around Monday, having included simple examples for all functions in stats and diagnostics.

@OriolAbril
Copy link
Member Author

Ready for review.

ColCarroll
ColCarroll approved these changes May 23, 2019
View reviewed changes
Copy link
Member

@ColCarroll ColCarroll left a comment

Choose a reason for hiding this comment

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

LGTM!

arviz/stats/diagnostics.py

Notes
-----
The basic ess diagnostic is computed by:

.. math:: \hat{N}_{eff} = \frac{MN}{\hat{\tau}}
.. math:: \hat{\tau} = -1 + 2 \sum_{t'=0}^K \hat{P}_t'
.. math:: \hat{\tau} = -1 + 2 \sum_{t'=0}^K \hat{P}_{t'}
Copy link
Member

Choose a reason for hiding this comment

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

nice catch

arviz/stats/diagnostics.py
@@ -311,7 +370,7 @@ def rhat(data, *, var_names=None, method="rank"):


def mcse(data, *, var_names=None, method="mean", prob=None):
"""Calculate Markov Chain Standard Error for statistic.
"""Calculate Markov Chain Standard Error statistic.
Copy link
Member

Choose a reason for hiding this comment

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

it'd be cool to have a reference for this eventually (but certainly not needed in this PR)

arviz/stats/stats.py Outdated Show resolved Hide resolved
arviz/stats/stats.py
...: "5%": lambda x: np.percentile(x, 5),
...: "median": lambda x: np.percentile(x, 50),
...: "95%": lambda x: np.percentile(x, 95),
...: }
Copy link
Member

Choose a reason for hiding this comment

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

this is nice

ahartikainen
ahartikainen reviewed May 23, 2019
View reviewed changes
Copy link
Contributor

@ahartikainen ahartikainen left a comment

Choose a reason for hiding this comment

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

Added a comment

arviz/stats/diagnostics.py Outdated
@@ -196,13 +202,13 @@ def ess(data, *, var_names=None, method="bulk", relative=False, prob=None):

.. ipython::

In [2]: az.ess(data, relative=True, var_names=["mu", "theta_t"])
In [1]: az.ess(data, relative=True, var_names=["mu", "theta_t"])

Calculate the ess using the "tail" method, which requires the `prob` argument
Copy link
Contributor

Choose a reason for hiding this comment

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

This is optional, by default it should not be used.

Because the default is what avehtari et al are using.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you prefer to remove it or to add a note advising against it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe remove it. I added it as an optional so one can do symmetrical minimum for different quantiles. Or maybe add a comment, but don't provide it in the example.

So if users thinks prob=0.1 is better then tail calculates symmetrical tail with that (min(ess_0.1, ess_1-0.1))

OriolAbril and others added 2 commits May 23, 2019 18:12
@OriolAbril @ColCarroll
Update arviz/stats/stats.py
9b90116 
Co-Authored-By: Colin <ColCarroll@users.noreply.github.com>
@OriolAbril
Remove tail example
0307adc
@ColCarroll ColCarroll merged commit 2b38527 into arviz-devs:master May 23, 2019
@ColCarroll
Copy link
Member

Thanks, @OriolAbril !

@OriolAbril OriolAbril deleted the stats-docs branch May 23, 2019 18:53
@ahartikainen
Copy link
Contributor

Hi, sorry to comment too late, but yes I think the tail method should be there, just the prob should not be touched? No need to reopen this PR, maybe quick fix with another one?

@OriolAbril
Copy link
Member Author

Hi, sorry to comment too late, but yes I think the tail method should be there, just the prob should not be touched? No need to reopen this PR, maybe quick fix with another one?

Oh, sorry, I missunderstood. I will fix it in #678. I wanted to merge this before getting into more lengthy work so that docs are updated as soon as possible.

@OriolAbril OriolAbril changed the title Stats docs Stats docs [GSoC] Aug 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahartikainen ahartikainen ahartikainen left review comments

@ColCarroll ColCarroll ColCarroll approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve effective sample size docs
4 participants
@OriolAbril @canyon289 @ahartikainen @ColCarroll