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

Convert stats and diagnostics to new API #232

Merged
merged 8 commits into from
Sep 16, 2018

Conversation

ColCarroll
Copy link
Member

These changes get the test suite for diagnostics.py to pass, with everything being xarray based. Now will need to work on stats.py, which will remove (most of) the pandas requirements. I am working on that, but will probably need help for some of the functions (#231) .

Note that most functions in diagnostics.py work with either a dataset, or a numpy array.

@ColCarroll ColCarroll changed the title [WIP] Convert stats and diagnostics to new API Convert stats and diagnostics to new API Sep 15, 2018
@ColCarroll
Copy link
Member Author

I think this is now ready to go. There's some missing documentation, but also this will allow us to delete most of utils.py. Note that compare and summary could now work with pystan objects, though I took out some of the arguments to make them simpler and maybe less flexible. The summary also does not look great, but it is computationally fine - I think this should go in, and we add an issue to improve pretty-printing.

image

image

raise ValueError('The number of observations should be the same '
'across all models')
raise ValueError(
"The number of observations should be the same " "across all models"
Copy link
Member

@canyon289 canyon289 Sep 16, 2018

Choose a reason for hiding this comment

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

Being picky, redundant parentheses here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed - I actually ran black on the file as an experiment - this might have been part of the result.

@ahartikainen
Copy link
Contributor

LGTM



__all__ = ['effective_n', 'gelman_rubin', 'geweke']


def effective_n(trace, varnames=None, round_to=2):
def effective_n(data, *, var_names=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

I didnt know you can use * as input - what does it do?

Copy link
Member

@canyon289 canyon289 Sep 16, 2018

Choose a reason for hiding this comment

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

It prevents people from using positional arguments, in this case in excess of data

@canyon289
Copy link
Member

LGTM as well

@canyon289 canyon289 merged commit db7ff57 into arviz-devs:master Sep 16, 2018
@ColCarroll ColCarroll deleted the xdiagnostics branch January 30, 2021 02:53
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.

None yet

4 participants