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

MLDA needs refactoring to become operable again #5021

Closed
michaelosthege opened this issue Sep 23, 2021 · 6 comments
Closed

MLDA needs refactoring to become operable again #5021

michaelosthege opened this issue Sep 23, 2021 · 6 comments

Comments

@michaelosthege
Copy link
Member

Description of your problem

In #5019 we removed the trace continuation feature, because it's making the logic in sampling.py considerably harder, particularly w.r.t. initial values.

The MLDA can probably be fixed easily by using pm.sample(start=...) and collecting the individual traces to concatenate them at the end.

@gmingas please check if you want to fix it.

Versions and main components

  • PyMC3 Version: main
michaelosthege added a commit to michaelosthege/pymc that referenced this issue Sep 23, 2021
michaelosthege added a commit to michaelosthege/pymc that referenced this issue Sep 24, 2021
@gmingas
Copy link
Contributor

gmingas commented Sep 27, 2021

Hi, thanks for pointing this out, sounds straightforward. @mikkelbue is maintaining this at the moment.

@mikkelbue
Copy link
Contributor

Hi
Thanks for the mention. I made use of trace continuation in the first place because it sped up sampling considerably. I think it was because it avoided creating and destroying new traces all the time, but I am not sure. Anyway, it is non-essential for the functionality.

@gmingas Would it break variance reduction if we just go back to the way you had implemented it in the first place, where it doesn't collect the full coarse chain?

I will have a look when I find some time, but I am trying to wrap up my PhD at the moment, so things are a bit tight.

@mikkelbue
Copy link
Contributor

mikkelbue commented Sep 28, 2021

Hi all. Just to let you know, we are working on this, but after solving this problem, a new issue has come up with the variance reduction feature, see alan-turing-institute#95.

@mikkelbue
Copy link
Contributor

Hi @michaelosthege
Just a short comment after running some tests. The trace continuation is not really the most important feature of the subsample function. They key is that it avoids a lot of the overhead of the pm.sample function. Essentially, it is a thin wrapper around _iter_sample() that skips all the tests and multi-chain tricks of pm.sample and _sample_many.

I tried using pm.sample, but using my refactored version of subsample is about 10x faster. So I would like to stick with the boiled-down version, but of course avoiding trace continuation. Unless there are some other breaking changes coming up about the architecture of the pm.sample and _iter_sample.

@michaelosthege
Copy link
Member Author

There are breaking charges on the lower-level functions, specifically around the start/initval kwarg. Overall the functions now require the dict to be complete and no longer run checks, so it should actually become a little faster. See #4983 and #5027

@mikkelbue
Copy link
Contributor

Thank you for your comments. Sorry for taking so long. I was waiting for the big PR you mentioned to be merged to be sure that this would fit in.

As you can see I have created a pull request with the required changes, and reintroduced the MLDA tests in test_step.py.

@twiecki twiecki closed this as completed Oct 23, 2021
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