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 !

[chandan5362]

Yeah sure @AlexAndorra , we can start working.
But, I am not very much familiar with rcParam and how are we going to use it for our purpose .
May be @OriolAbril could help me out here a little.
Also, from where should we start?

[OriolAbril]

Pymc3 does not have rcparams (yet?), if we want to use that instead of setting return_inferencedata=True we can set something up. We can use arviz functions to generate the class so it should not be too much code nor too much work but not sure it is worth it.

Are there other params that could benefit from that? If it's only want it does not make much sense

[chandan5362]

if we want to use that instead of setting return_inferencedata=True we can set something up. We can use arviz functions to generate the class so it should not be too much code nor too much work but not sure it is worth it.

Are you @OriolAbril referring to from_pymc3 function to generate InferenceData?

[OriolAbril]

I see 3 main options:

• Use return_inferencedata=True: Allows updating everything effective immediate, with the drawback of having to go back to each pm.sample instance at some indeterminate point in the future to remove the argument (I am assuming there will be 2 steps, first setting the default to True and afterwards delete the argument)
• Add a pymc3.return_inferencedata to arviz.rcParams. ArviZ is a dependency of PyMC3, so ArviZ rcParams are always available to pymc and can be read to set pymc defaults. Allows updating everything starting with next ArviZ release (which could be a patch release to accelerate releasing this) and would only require one change per file/module (depends on how they are executed). For example, setting it in conf.py will change the default of all ipython/plot directives (not sure there are any though), setting in a notebook will set the default for the whole notebook. Downside, it is kind of dirty and probably confusing for users.
• Add a pymc3.rcParams with a return_inferencedata, mcmc.return_inferencedata, sample.return_inferencedata... key. Allows updating as soon as the pymc3.rcParams pr is merged and would also require a change per file/module. Downside, even though class constructors and validators can be imported from ArviZ, it is probably an overkill to add that feature for a single parameter. It may be worth it if there were a desire to add more parameters (not sure what could be added though, tuning method? default number of draws?)

[chandan5362]

@AlexAndorra, what should we do ?

[AlexAndorra]

I wonder if option 1 is not the way to go, since it should be somewhat easy to do the change in one go with an IDE 🤔
Also, I don't remember if we're planning to just switch the default return type, or also completely deprecate the MultiArray backend -- do you, @michaelosthege ?

[michaelosthege]

@AlexAndorra @chandan5362 Sorry I did not notice that this conversation was going on.

There are 99 occurences of .sample( in our test suite.

1. This is the option I had in mind so far. 99 occurrences is just fine to modify in a single commit. And taking out the explicit kwarg at some point in the future is also fine.
2. I don't like this option. Mixing config between libraries is confusing. Also aesara.config would be an alternative and can do contextually changing the setting.
3. We can use a rcParams (or aesara.ConfigParser) anyways, for example to host the Model(check_bounds=...) setting.

I think we can do both - Option 1 in #4446 and adding pymc3.rcParams or pymc3.config is independent of that.

As much as I would like to get rid of the MultiTrace backend, I don't think we can do this in the near future. Defaulting to return_inferencedata=True comes first and hopefully after re-writing the PyMC3 internals for RandomVariable we can reconsider our trace backend.

[chandan5362]

@AlexAndorra @michaelosthege So, I am proceeding with option 1. let's see how much time does it take?
Also, once we modify the tests. we will have take care of it in the documentations as well. So, How are we planning deal with?

[chandan5362]

Just to get on the pace, How much time do we have to complete this PR before rolling out to the users?

[michaelosthege]

@chandan5362 hold on - in our PyMC meeting yesterday we discussed about this topic. We concluded that because PyMC3 4.0 is getting closer, we don't need to switch the default in a PyMC3 3.x version.

However, many tests rely on the current default of return_inferencedata=False and so do some examples in https://github.com/pymc-devs/pymc-examples.

I think that some of them are currently not compatible with return_inferencedata=True and we'll need to find out which of them are.
In order to avoid git conflicts as much as possible, I think the best way for you to contribute on this issue is to go through example notebooks in https://github.com/pymc-devs/pymc_examples and try to refactor them to use return_inferencedata=True as much as possible. This will help to identify limitations and make sure that all central functionality is fully compatible.

Sorry about this back and forth. The Timeline right now is somewhat delicate ;)

[chandan5362]

It's okay @michaelosthege , No problem at all.
Though above links are not working, I will try refactoring the notebooks stored in https://github.com/pymc-devs/pymc-examples .