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

Unified Infer function for marginal inference (with single options argument) #86

Closed
stuhlmueller opened this issue Apr 13, 2015 · 17 comments
Assignees
Milestone

Comments

@stuhlmueller
Copy link
Member

Options can then be an object such as:

{
  numParticles: 100,
  rejuvSteps: 1000
}
@ngoodman
Copy link
Contributor

In the interest of unifying the interface, and rather than breaking the current fns, I'd prefer to see a new function Marginal(foo, options) that takes a thunk foo and takes an options object, where the options object includes both a method and method-specific options: {method: "Enumerate", depth: 100}.

@ngoodman
Copy link
Contributor

Btw. it would be neat if the options structure for SMC was somewhat compositional. E.g.:

{
  method: "Enumerate",
  numParticles: 100,
  rejuvenation: {method: "mcmcmix", 
                 kernels: [{method: "MH", iterations: 10}, 
                           {method: "HMC", leapfrogsteps: 5}]}
}

@null-a
Copy link
Member

null-a commented Apr 17, 2015

Slight variation on the theme:

Marginal("MH", function() {
...
}, {steps: 100})

This might make it easier to pick out the method/algorithm, especially when passing the model as an anonymous function.

At the risk of revealing my ignorance, I admit that when I first looked at webppl I was confused by the way in which Enumerate and friends are described as marginalization operations - it was conditioning or inference that I was looking to perform. I suppose I would have been less confused by something like infer("MH", function() {...}) at the time.

I get that this doesn't really work when you don't have factor statements, but that might be a price worth paying in order to make things easier for new users. (This assumes my experience is typical, which isn't clear. Thought this was worth sharing though!)

@ngoodman
Copy link
Contributor

Good point: probably we should call it InferMarginal(...), and we could alias to Marginal and Infer for convenience.

By the same token, factor is actually not an obvious word to those who aren't used to factor graphs... We could have an alias like WeightExecution or something. And we should definitely add condition(expr) as an alias for factor(expr?0:-Infinity).

I prefer putting the method in the options object, because even though it isn't quite as easy to pick out the algroithm, it better supports the compositional descriptions of how to infer.

@dritchie
Copy link
Contributor

dritchie commented May 8, 2015

Just wanted to weigh in while this is still open:

If we're changing the interface to inference algs, I'd appreciate it if the individual sampling functions (e.g. MH, ParticleFilter) returned a list of samples. Calls to these functions could be abstracted behind a function Marginal (or InferMarginal, or whatever) that converts the sample list into a histogram / marginalized ERP.

My reasoning here is that the current scheme (i.e. all of the sampling functions returning marginalized ERPs directly) is great for discrete inference problems, but maybe not so much for continuous ones, and especially not for programs that return giant data structures (e.g. procedural models). In these cases, having the option to go straight to the raw samples can be much nicer. As things stand, I currently have to hack every single sampling function I want to use to give it an option to just store the raw samples--ugly, and results in a lot of repeated code.

@null-a
Copy link
Member

null-a commented Aug 12, 2015

especially not for programs that return giant data structures

Is this just a side-effect of us doing JSON.stringify on return values? I'm imagining that if we avoided this, them the over head of maintaining counts doesn't seem so bad, and it would be possible to reconstruct samples from the counts after inference should they be required.

@stuhlmueller
Copy link
Member Author

and it would be possible to reconstruct samples from the counts after inference should they be required.

For algorithms like MH, you'd still need to store the order of the samples somewhere, right? Otherwise, you can't run convergence diagnostics.

@null-a
Copy link
Member

null-a commented Aug 14, 2015

Otherwise, you can't run convergence diagnostics.

I'd not considered that, though I get the impression that this isn't @dritchie's motivation for wanting samples. (Which is the thing I'm trying to understand.)

If this is the case, I wonder if it might be handled through callbacks/hooks. So we might have things like:

Marginal(model, { method: MCMC, kernel: MH })
MAP(model, { method: MCMC, kernel: MH, callback: GelmanRubin })

@dritchie suggests an alternative here.

@stuhlmueller
Copy link
Member Author

Besides keeping track of sample order, another potential motivation for having samples instead of counts may be the fear that alternative ways for getting unique ids (~ hashes) from big data structure samples will be comparably inefficient to JSON.stringify.

@null-a
Copy link
Member

null-a commented Nov 5, 2015

HMC (#81) could use the ability to specify per-kernel options, and this probably requires implementing at least some of the interface Noah proposes here.

I can think of two ways of doing this which are within easy reach of the current implementation. First something like this:

SMC(model, { rejuvKernel: 'MH' })
SMC(model, { rejuvKernel: { HMC: { stepSize: 0.01 } } })
MCMC(model, { kernel: { sequence: [{ MH: { discreteOnly: true } }, 'HMC'] } })

The second, something like:

SMC(model, { rejuvKernel: MH() })
SMC(model, { rejuvKernel: { HMC({ stepSize: 0.01 }) })
MCMC(model, { kernel: { sequence(MH({ discreteOnly: true }), HMC()) } })

This second approach is a little bit problematic because we already have a MH function, and having a function call when there are no options to pass (e.g. MH()) is a bit odd. These things are fixable though if we liked it otherwise.

Are either of these reasonable things to implement for now, or is there something better we can do which moves us closer to the interface we'd like for Marginal?

@ngoodman
Copy link
Contributor

ngoodman commented Nov 5, 2015

i like the first approach, as it is closer to what i have in mind for the
general Marginal (/Infer) interface.

-N

On Thu, Nov 5, 2015 at 10:43 AM, null-a notifications@github.com wrote:

HMC (#81 #81) could use the
ability to specify per-kernel options, and this probably requires
implementing at least some of the interface Noah proposes here.

I can think of two ways of doing this which are within easy reach of the
current implementation. First something like this:

SMC(model, { rejuvKernel: 'MH' })SMC(model, { rejuvKernel: { HMC: { stepSize: 0.01 } } })MCMC(model, { kernel: { sequence: [{ MH: { discreteOnly: true } }, 'HMC'] } })

The second, something like:

SMC(model, { rejuvKernel: MH() })SMC(model, { rejuvKernel: { HMC({ stepSize: 0.01 }) })MCMC(model, { kernel: { sequence(MH({ discreteOnly: true }), HMC()) } })

This second approach is a little bit problematic because we already have a
MH function, and having a function call when there are no options to pass
(e.g. MH()) is a bit odd. These things are fixable though if we liked it
otherwise.

Are either of these reasonable things to implement for now, or is there
something better we can do which moves us closer to the interface we'd like
for Marginal?


Reply to this email directly or view it on GitHub
#86 (comment).

@stuhlmueller
Copy link
Member Author

Agreed, I think it's fine to go ahead and implement the first approach.

@null-a null-a added this to the v0.8 milestone Feb 12, 2016
@ngoodman ngoodman modified the milestones: v0.7, v0.8 Feb 18, 2016
@ngoodman ngoodman changed the title Replace multiple arguments to inference algorithms with single options argument Unified Infer function for marginal inference (with single options argument) Feb 18, 2016
@null-a
Copy link
Member

null-a commented Feb 22, 2016

From Noah:

andreas and i had the idea that there should be some sane heuristic used when​ the 'options' are omitted. something like: try rejection for a second or two and see if you get any samples (if so it's probably a good strategy), then try enumeration for a bit and see how badly the queue is exploding, then fall back on SMC+rejuv.

Update: Now #437

@null-a
Copy link
Member

null-a commented Apr 18, 2016

My plan for the initial implementation of this is to add a new function to the header which simply looks at options.method and delegates to the appropriate coroutine. I think the heuristics for the case where options is missing should be a separate piece of work.

This ends up looking like:

Infer(thunk, {method: 'MCMC', kernel: 'HMC', samples: 10})
Infer(thunk, {method: 'SMC', rejuvSteps: 1})
Infer(thunk, {method: 'Enumerate', maxExecutions: 10})
Infer(thunk, {method: 'IncrementalMH', samples: 100, justSample: true})
// etc.

You can see a work-in-progress version of this here.

The only small complication I've run into so far is the fact that we need to update the check we make to decide whether to run the caching transform. At present this simply looks for the identifier IncrementalMH, so I've updated it to also look for an object literal containing {method: 'IncrementalMH'} as a simple way to catch the use of IncrementalMH via Infer.

In the interest of unifying the interface, and rather than breaking the current fns

@ngoodman: Do you still think it's preferable not change the interface of the current inference methods? The implementation of Infer would be much cleaner if all inference methods took a thunk and some options, and perhaps it's worth taking the hit so that things are nice and tidy for v1?

@null-a
Copy link
Member

null-a commented May 14, 2016

I'm a little bit concerned about the readability of the current approach when using an anonymous function for the program/model:

Infer(function() {
  //
  //
  // 
  // many more lines of code...
}, {method: 'MCMC'})

For any given call to Infer, it seems like we'll often want to know the details of how inference will be performed. Separating Infer from its options seems like it will make figuring that out harder than it needs to be.

I wonder if we should flip the arguments:

Infer({method: 'MCMC'}, function() {
  //
  //
  //
  // many more lines of code
});

This might get a bit messy when there are lots of options, but something like the following seems tolerable:

Infer({
  method: 'MCMC'
  samples: 10,
  burn: 10,
  lag: 2,
  kernel: 'HMC'        
}, function() {
  //
  //
  //
  // many more lines of code
});

@ngoodman
Copy link
Contributor

i agree -- let's switch the order of args to Infer. (if you submit a pr with just this i can review / merge.)

@null-a
Copy link
Member

null-a commented May 15, 2016

if you submit a pr with just this i can review / merge

Now #433.

The only snag with this signature for Infer that I can think of is that once we have heuristics for automatically selecting an inference strategy, omitting the options arg is a tiny bit messier than it would otherwise be. Though we can still make Infer(thunk) work of course.

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

No branches or pull requests

4 participants