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

sample method modifies start dictionary #4456

Closed
lhelleckes opened this issue Feb 2, 2021 · 6 comments
Closed

sample method modifies start dictionary #4456

lhelleckes opened this issue Feb 2, 2021 · 6 comments

Comments

@lhelleckes
Copy link
Contributor

When specifying start in the sample method, the provided dictionary is modified

Code example

start_dict = {
    'X0_mu': 25
}
with pymc3.Model() as pmodel:
    X0_mu = pymc3.Lognormal('X0_mu', mu=numpy.log(0.25), sd=0.10)
    idata = pymc3.sample(
        tune=100, draws=500,
        start=start_dict,
    )
    
print(start_dict)

Output

{'X0_mu': 25, 'X0_mu_log__': 3.2188758248682006}

Expected behaviour
The method should edit a copy of the original dictionary for reproducibility.

Versions and main components

  • PyMC3 Version: 3.11.0
  • Python Version: 3.7
@chandan5362
Copy link
Contributor

chandan5362 commented Feb 2, 2021

@lhelleckes , need a little more information,
the output should be only X0_mu : 25 right?

@ricardoV94
Copy link
Member

ricardoV94 commented Feb 2, 2021

@lhelleckes , need a little more information,
the output should be only X0_mu : 25 right?

Yes, I am pretty sure that's it. We should make a copy before update_start_vals: https://github.com/pymc-devs/pymc3/blob/37ca5ea9b25aa00ed4d460f0fb417712d125d248/pymc3/sampling.py#L428-L433

@chandan5362 Do you want to make a PR?

@michaelosthege
Copy link
Member

pm.find_MAP possibly has the same issue.

@michaelosthege michaelosthege added this to the vNext (3.11.1) milestone Feb 2, 2021
@chandan5362
Copy link
Contributor

chandan5362 commented Feb 2, 2021

The main factor behind this issue (for both pm.find_MAP , pm.sample) is the function update-start-vals, as it updates the passed arguments (includes transformed variable as well). As It can be seen here.
https://github.com/pymc-devs/pymc3/blob/b77e2d1498a74872ccabc8ea9f51add447e41941/pymc3/util.py#L182-L193
so, one way to get rid of this issue is to use deepcopy (if you agree) or we can go for creating extra variable ( I don't like it though).
If you all agree, then I can create a PR.

@michaelosthege
Copy link
Member

Wouldn't it be enough to just re-create the dict like start = { k : v for k, v in start.items() } somewhere in pm.sample and possibly pm.find_MAP?

But you can't do that in update_start_vals because it was designed to modify the pass-by-reference input and has no return value. Unless you change that too.

@ricardoV94
Copy link
Member

You also have to handle the case where you have a list of dictionaries (one per chain)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants