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

Fix az.from_numpyro crash when running MCMC with thinning > 1 #1619

Merged
merged 10 commits into from
Mar 19, 2021

Conversation

kpj
Copy link
Contributor

@kpj kpj commented Mar 18, 2021

Description

This PR fixes the crash observed when running the following code:

import arviz as az
import numpy as np
import numpyro
import numpyro.distributions as dist
from jax import random
from numpyro.infer import MCMC, NUTS


data = np.random.normal(10, 3, size=100)

def model(data):
    numpyro.sample(
        'x',
        dist.Normal(
            numpyro.sample('loc', dist.Uniform(0, 20)),
            numpyro.sample('scale', dist.Uniform(0, 20)),
        ),
        obs=data,
    )


kernel = NUTS(model)
mcmc = MCMC(NUTS(model), 100, 200, thinning=2)

mcmc.run(random.PRNGKey(0), data=data)
mcmc.print_summary()

az.from_numpyro(mcmc)  # crash

Fixes #1618 (line edited by @OriolAbril to use github issue closing keywords)

Checklist

  • Follows official PR format
  • Includes new or updated tests to cover the new feature
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

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.

Hi, added couple of comments. Looks good.

arviz/tests/external_tests/test_data_numpyro.py Outdated Show resolved Hide resolved
arviz/tests/external_tests/test_data_numpyro.py Outdated Show resolved Hide resolved
arviz/tests/external_tests/test_data_numpyro.py Outdated Show resolved Hide resolved
arviz/tests/external_tests/test_data_numpyro.py Outdated Show resolved Hide resolved
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.

Thanks for the superfast PR already with tests! ❤️

arviz/data/io_numpyro.py Outdated Show resolved Hide resolved
@kpj
Copy link
Contributor Author

kpj commented Mar 19, 2021

Thanks for your great suggestions :-)

I implemented them and the CI/CD errors do not seem to come from my code.

@ahartikainen
Copy link
Contributor

Ok, these are not related to your code


arviz/data/inference_data.py:1630 in public function `concat`:
        D418: Function/ Method decorated with @overload shouldn't contain a docstring
arviz/data/inference_data.py:1642 in public function `concat`:
        D418: Function/ Method decorated with @overload shouldn't contain a docstring
arviz/data/inference_data.py:1655 in public function `concat`:
        D418: Function/ Method decorated with @overload shouldn't contain a docstring
arviz/data/inference_data.py:1668 in public function `concat`:
        D418: Function/ Method decorated with @overload shouldn't contain a docstring
arviz/data/inference_data.py:1681 in public function `concat`:
        D418: Function/ Method decorated with @overload shouldn't contain a docstring

I will quickly fix in in another PR.

@ahartikainen ahartikainen merged commit 8d3a615 into arviz-devs:main Mar 19, 2021
@ahartikainen
Copy link
Contributor

Thanks for the PR

utkarsh-maheshwari pushed a commit to utkarsh-maheshwari/arviz that referenced this pull request May 27, 2021
…viz-devs#1619)

* Fix crash when importing numpyro model with thinning > 1

* Format code

* Add test for numpyro import with thinning

* Add entry to CHANGELOG.md

* Update link to PR in CHANGELOG.md

* Give model tested in 'test_mcmc_with_thinning' a more generic name

* Test more MCMC parameters in 'test_mcmc_with_thinning'

* Test correct quantity in 'test_mcmc_with_thinning'

* Test 'test_mcmc_with_thinning' with multiple chain counts

* Simplify implementation of numpyro thinning fix
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.

az.from_numpyro crashes when running MCMC with thinning != 1
3 participants