Add None for fields not used in beanmachine converter

[zaxtax]

Description

This is a bug fix to handle when observations and posterior_predictive is missing from the MonteCarloSamples object

Checklist

• Follows official PR format
• Includes a sample plot to visually illustrate the changes (only for plot-related functions)
• New features are properly documented (with an example if appropriate)?
• Includes new or updated tests to cover the new feature
• Code style correct (follows pylint and black guidelines)
• Changes are listed in changelog

---

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

[codecov]

Codecov Report

Merging #2154 (313133a) into main (e87d9e3) will increase coverage by 2.22%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##             main    #2154      +/-   ##
==========================================
+ Coverage   87.73%   89.95%   +2.22%     
==========================================
  Files         119      119              
  Lines       12394    12398       +4     
==========================================
+ Hits        10874    11153     +279     
+ Misses       1520     1245     -275     

Impacted Files	Coverage Δ
arviz/data/io_beanmachine.py	92.68% <25.00%> (+54.84%)	⬆️
arviz/data/inference_data.py	83.05% <0.00%> (+0.13%)	⬆️
arviz/data/base.py	98.21% <0.00%> (+0.59%)	⬆️
arviz/data/io_emcee.py	98.29% <0.00%> (+2.56%)	⬆️
arviz/data/io_pystan.py	96.89% <0.00%> (+45.70%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[OriolAbril]

Can you also add some tests or an issue so we remember we need to add them at some point?

[zaxtax]

I'll add them to this issue. It needs to be done.

…

On Sat, 5 Nov 2022, 22:56 Oriol Abril-Pla, ***@***.***> wrote: Can you also add some tests or an issue so we remember we need to add them at some point? — Reply to this email directly, view it on GitHub <#2154 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACCUNWSF7LAON7CBMVUSTWG3JXZANCNFSM6AAAAAARWNI5SM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[zaxtax]

Added tests

[OriolAbril]

Thanks! I have added some comments to try and use check_multiple_attrs if possible so tests are in line with the other libraries. But I am not sure if they'd be equivalent.

[zaxtax]

I couldn't use `check_multiple_attrs` since it assumes the variables are strings. I could generalise it, but wasn't sure if that's the best option.

…

On Tue, 6 Dec 2022, 12:06 Oriol Abril-Pla, ***@***.***> wrote: ***@***.**** commented on this pull request. Thanks! I have added some comments to try and use check_multiple_attrs if possible so tests are in line with the other libraries. But I am not sure if they'd be equivalent. ------------------------------ In arviz/tests/external_tests/test_data_beanmachine.py <#2154 (comment)>: > @@ -0,0 +1,77 @@ +# pylint: disable=no-member, invalid-name, redefined-outer-name +import numpy as np +import pytest + +from ...data.io_beanmachine import from_beanmachine # pylint: disable=wrong-import-position +from ..helpers import ( # pylint: disable=unused-import, wrong-import-position + chains, + check_multiple_attrs, + draws, + eight_schools_params, + importorskip, + load_cached_models, +) + +# Skip all tests if pyro or pytorch not installed ⬇️ Suggested change -# Skip all tests if pyro or pytorch not installed +# Skip all tests if beanmachine or pytorch not installed ------------------------------ In arviz/tests/external_tests/test_data_beanmachine.py <#2154 (comment)>: > + assert mu in inference_data.posterior + assert tau in inference_data.posterior + assert eta in inference_data.posterior + assert obs in inference_data.posterior_predictive ⬇️ Suggested change - assert mu in inference_data.posterior - assert tau in inference_data.posterior - assert eta in inference_data.posterior - assert obs in inference_data.posterior_predictive + test_dict = { + "posterior": ["mu", "tau", "eta"], + # or + # "posterior": [mu, tau, eta], ? + "posterior_predictive": ["obs"], + # if we also want to assert other groups are missing + "~observed_data": [], + } + fails = check_multiple_attrs(test_dict, inference_data) + assert not fails ------------------------------ In arviz/tests/external_tests/test_data_beanmachine.py <#2154 (comment)>: > + assert obs in idata.log_likelihood + assert obs in idata.observed_data ⬇️ Suggested change - assert obs in idata.log_likelihood - assert obs in idata.observed_data + test_dict = { + "observed_data": ["obs"], + "log_likelihood": ["obs"], + } + fails = check_multiple_attrs(test_dict, idata) + assert not fails ------------------------------ In arviz/tests/external_tests/test_data_beanmachine.py <#2154 (comment)>: > + assert not model.obs() in inference_data.posterior + assert "observed_data" not in inference_data ⬇️ Suggested change - assert not model.obs() in inference_data.posterior - assert "observed_data" not in inference_data + test_dict = { + "posterior": ["~obs"], + "~observed_data": [], + } + fails = check_multiple_attrs(test_dict, inference_data1) + assert not fails — Reply to this email directly, view it on GitHub <#2154 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACCUIZ2TKI6OFK6DGCOULWL4M3XANCNFSM6AAAAAARWNI5SM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[OriolAbril]

let's merge like this then, the goal of check_multiple_attrs is making it easier to write extensive tests but now they are already written and they do what they are supposed to do. Can you add to the changelog and fix the two unresolved comments? Otherwise I'll try to do it later today.