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

Move pointwise log likelihood data to log_likelihood group (Pyro + NumPyro) #1044

Merged
merged 10 commits into from
Feb 6, 2020

Conversation

VincentBt
Copy link
Contributor

Follows #796, for Pyro and NumPyro: I added "log_likelihood" to InferenceData, and added tests for multiple observed variables.

Thanks for a Pyro/NumPyro expert to review this (@fehiepsi maybe?). Indeed, I am not entirely sure about the Pyro part: the test_multiple_observed_rv fails if y2 and y1 have different dimensions.
For the rest, I copied io_pymc3.py, in particular for coord_name and dims.

Sorry in advance for the possible mistakes, it's my first commit.

Copy link
Member

@fehiepsi fehiepsi 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 PR! Overall looks good to me (comparing to io_pymc3). I think you would need to fix lint errors too: I use pylint arviz and black arviz as explained in the guide.

the test_multiple_observed_rv fails if y2 and y1 have different dimensions

Pyro tensor shapes are quite restricted. I think you would need some plate statement:

with pyro.plate('plate_y1', 10):
    pyro.sample('y1', dist.Normal(x, 1), obs=y1)

arviz/data/io_pyro.py Outdated Show resolved Hide resolved
arviz/data/io_pyro.py Outdated Show resolved Hide resolved
arviz/tests/test_data_numpyro.py Outdated Show resolved Hide resolved
arviz/tests/test_data_numpyro.py Outdated Show resolved Hide resolved
arviz/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 PR! LGTM

I only have a question out of curiosity about Pyro try except. Is it possible to have some observations where the vectorized likelihood cannot be accessed and some where it can?

In case the try except must be a block as it is now, the pass should probably be a return None, as it would not make much sense to have an empty group in an InferenceData, if empty the group should not be present

@OriolAbril
Copy link
Member

Can you update also Azure configuration file (in the same way as Travis) and add this to the changelog?

Everything else looks good, after these last changes we can merge

@VincentBt
Copy link
Contributor Author

In case the try except must be a block as it is now, the pass should probably be a return None

You're right, I changed the code as you suggested.

Ready to merge.

PS: is it possible to run the Travis and Azure checks on my computer for the specific files that changed? That would have decreased the number of unnecessary commits (not talking about having to wait for all the checks to finish on this page before getting a failure)

@OriolAbril
Copy link
Member

Travis and Azure mainly run the tests with pytest and the linters. If you have the required dependencies installed, you can run pytest arviz/tests/test_file.py or pytest arviz/tests -k "key1 and key2 or key3" to tests only a subset of the files/test functions. In addition, running .scripts/lint.sh should mimic the lint job.

If you want to keep special versions in your environment you can always use docker to run tests locally being sure the dependencies installed will be the same as the ones in Azure and Travis first job. There is a section on this in the contributing guide. The --test flag is a shortcut to run all tests, to run only some of them, enter a bash session in docker and run the specific pytest command.

Finally, the documentation is also generated automatically and sphinx (or sphinx inside docker) can be used to build it locally and check everything works. In many cases it may not be necessary.

@OriolAbril OriolAbril merged commit a82ae30 into arviz-devs:master Feb 6, 2020
@OriolAbril
Copy link
Member

Thanks for the PR!

@VincentBt VincentBt deleted the numpyro_PR_794 branch February 7, 2020 17:16
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