Make `pm.sample` return `InferenceData` by default

[AlexAndorra]

The return_inferencedata kwarg has been there for a while in pm.sample, without big bugs appearing, so we should add a DeprecationWarning in 3.11, saying that the default format will change to InferenceData in the next major release, aka 4.0.0.
Feel free to comment if you wanna take on this issue 🖖

[chandan5362]

Hi @AlexAndorra .
As far as I know, currently it returns MultiTrace by default and it raises FutureWarning if no argument is passed to return_inferencedata .
So, Do you mean to change the FutureWarning to DeprecationWarning ?

[AlexAndorra]

Hi @chandan5362 ! Yes, exactly. And then actually implementing the default switch in the code -- and see whether our tests suite still passes. You wanna take that on?

[chandan5362]

You wanna take that on?

Yeah! sure, I would love to.
I will only have to change the warning section of the given block.

if return_inferencedata is None:
        v = packaging.version.parse(pm.__version__)
        if v.release[0] > 3 or v.release[1] >= 10:  # type: ignore
            warnings.warn(
                "In an upcoming release, pm.sample will return an `arviz.InferenceData` object instead of a `MultiTrace` by default. "
                "You can pass return_inferencedata=True or return_inferencedata=False to be safe and silence this warning.",
                FutureWarning,
            )
        # set the default
        return_inferencedata = False

am I going good?

[AlexAndorra]

Hi @chandan5362,
Yep. Although I was mainly talking about the second part, which is really the crux of the PR: actually implementing the default switch in the code -- and see whether our tests suite still passes

[chandan5362]

Yep. Although I was mainly talking about the second part, which is really the crux of the PR: actually implementing the default switch in the code -- and see whether our tests suite still passes

You meant setting return_inferencedata default to True?

[OriolAbril]

I got a little lost in hypothetical changes. TL;DR I think we should start updating the code to return_inferencedata=True as soon as possible and once it is done or mostly done, change the default. I will probably be weird to have a deprecation error and still have plenty of official docs triggering it, even more so to change the default and continue having plenty of docs use MultiTrace.

Yeah, changing the default to True will definitely break the code in several places, maybe not only the tests. It is not clear how much work will it require but it will probably mean going over the whole library updating calls to pm.sample. My guess would be that tests will require a significant amount of job, the library itself should not be too time consuming (if any change is needed at all) and documentation will be also a hard job.

I was thinking that maybe we could already start working on it using rcParams or something like that. As I understand it, the final goal is to remove return_inferencedata and have objects always return inferencedata, so changing everything to use return_inferencedata=True would probably be useful but its not ideal because it requires a 2nd comb of the library on the long run to remove these arguments. However, using an rcParam could reduce the 2nd comb to a line per file or so.

To use an example, updating https://docs.pymc.io/notebooks/multilevel_modeling.html to use inferencedata instead of multitrace required considerable work, and the more specific the notebook the more work it will be (basically everything that differs from az.plot... or az.summary will have to be rewritten!). But eventually it will still require to go through each occurrence of pm.sample and remove return_inferencedata.

Also, these updates will allow us to write moving from MultiTrace to InferenceData guidance. For example, something like this:

idata = pm.sample(...)
trace = idata.posterior

or

idata = pm.sample(...)
trace = idata.posterior.stack(sample=("chain", "draw"))

can make "multitrace code" like trace["variable_name"] still work

[chandan5362]

I totally agree with @OriolAbril .
In fact, many of the test suits have been written considering MultiTrace attributes.
Just for the reference, I am attaching the tests logs here.

============================================================= short test summary info ==============================================================
FAILED pymc3/tests/test_sampling.py::TestSample::test_parallel_sample_does_not_reuse_seed - AttributeError: 'InferenceData' object has no attribu...
FAILED pymc3/tests/test_sampling.py::TestSample::test_parallel_start - AttributeError: 'InferenceData' object has no attribute 'get_values'
FAILED pymc3/tests/test_sampling.py::TestSample::test_sample_tune_len - TypeError: object of type 'InferenceData' has no len()
FAILED pymc3/tests/test_sampling.py::TestSample::test_trace_report[True-NUTS] - AttributeError: 'InferenceData' object has no attribute 'report'
FAILED pymc3/tests/test_sampling.py::TestSample::test_trace_report[True-Metropolis] - AttributeError: 'InferenceData' object has no attribute 're...
FAILED pymc3/tests/test_sampling.py::TestSample::test_trace_report[True-Slice] - AttributeError: 'InferenceData' object has no attribute 'report'
FAILED pymc3/tests/test_sampling.py::TestSample::test_trace_report[False-NUTS] - AttributeError: 'InferenceData' object has no attribute 'report'
FAILED pymc3/tests/test_sampling.py::TestSample::test_trace_report[False-Metropolis] - AttributeError: 'InferenceData' object has no attribute 'r...
FAILED pymc3/tests/test_sampling.py::TestSample::test_trace_report[False-Slice] - AttributeError: 'InferenceData' object has no attribute 'report'
FAILED pymc3/tests/test_sampling.py::TestSample::test_trace_report_bart - AttributeError: 'InferenceData' object has no attribute 'report'
FAILED pymc3/tests/test_sampling.py::TestSample::test_sampler_stat_tune[1] - AttributeError: 'InferenceData' object has no attribute 'get_sampler...
FAILED pymc3/tests/test_sampling.py::TestSample::test_sampler_stat_tune[2] - AttributeError: 'InferenceData' object has no attribute 'get_sampler...
FAILED pymc3/tests/test_sampling.py::TestSample::test_callback_can_cancel - TypeError: object of type 'InferenceData' has no len()
FAILED pymc3/tests/test_sampling.py::TestSamplePPC::test_normal_scalar - AttributeError: 'InferenceData' object has no attribute 'report'
FAILED pymc3/tests/test_sampling.py::TestSamplePPC::test_normal_vector - AttributeError: 'InferenceData' object has no attribute 'nchains'
FAILED pymc3/tests/test_sampling.py::TestSamplePPC::test_model_shared_variable - TypeError: 'InferenceData' object is not subscriptable
FAILED pymc3/tests/test_sampling.py::TestSamplePPC::test_deterministic_of_observed - TypeError: object of type 'InferenceData' has no len()
FAILED pymc3/tests/test_sampling.py::TestSamplePPC::test_deterministic_of_observed_modified_interface - AttributeError: 'InferenceData' object ha...
FAILED pymc3/tests/test_sampling.py::TestSamplePosteriorPredictive::test_point_list_arg_bug_fspp - TypeError: 'InferenceData' object is not subsc...
FAILED pymc3/tests/test_sampling.py::TestSamplePosteriorPredictive::test_point_list_arg_bug_spp - TypeError: 'InferenceData' object is not subscr...
FAILED pymc3/tests/test_sampling.py::TestSamplePosteriorPredictive::test_sample_from_xarray_prior - AttributeError: 'InferenceData' object has no...
FAILED pymc3/tests/test_sampling.py::TestSamplePosteriorPredictive::test_sample_from_xarray_posterior - AttributeError: 'InferenceData' object ha...
FAILED pymc3/tests/test_sampling.py::TestSamplePosteriorPredictive::test_sample_from_xarray_posterior_fast - AttributeError: 'InferenceData' obje...
============================================= 23 failed, 60 passed, 712 warnings in 489.86s (0:08:09) ==============================================

These failures only account for test_sampling.py.
I am expecting multiple test suits failure if I go for entire codebase.

[chandan5362]

what do you say @AlexAndorra ?

[AlexAndorra]

Yeah, I agree with @OriolAbril's roadmap 👌 Especially:

I was thinking that maybe we could already start working on it using rcParams or something like that. As I understand it, the final goal is to remove return_inferencedata and have objects always return inferencedata, so changing everything to use return_inferencedata=True would probably be useful but its not ideal because it requires a 2nd comb of the library on the long run to remove these arguments. However, using an rcParam could reduce the 2nd comb to a line per file or so.

If that all sounds clear to you, I think we can start @chandan5362 !