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

Pass Cholesky to multivariate gaussian #5

Merged
merged 4 commits into from
Nov 18, 2013
Merged

Pass Cholesky to multivariate gaussian #5

merged 4 commits into from
Nov 18, 2013

Conversation

jucor
Copy link
Contributor

@jucor jucor commented Nov 18, 2013

Issue by jucor from Friday Nov 01, 2013 at 18:30 GMT
Originally opened as google-deepmind/torch-randomkit#5


@jucor
Copy link
Contributor Author

jucor commented Nov 18, 2013

Comment by jucor from Saturday Nov 09, 2013 at 08:15 GMT


The more I think of it, the more I want it :-)
Either as a separate function, or an optional options table as the last argument. That complicates a bit the argument passing, but that should be doable, since the covariance is never a table.

@jucor
Copy link
Contributor Author

jucor commented Nov 18, 2013

Comment by d11 from Monday Nov 11, 2013 at 10:36 GMT


Hmm, if we went with the latter, it'd look like the following, right?

multivariateGaussianRand( n | resultTensor, mu, { cholesky = myChol })

another option could be

multivariateGaussianRand( n | resultTensor, mu, cholesky, true )

where the last argument is a flag indicating that the covariance matrix is already decomposed…
I'm not sure what I favour for this, really… which is more torch-y? Has anyone given a preference about how to call it?

On 9 Nov 2013, at 08:15, Julien Cornebise notifications@github.com wrote:

The more I think of it, the more I want it :-)
Either as a separate function, or an optional options table as the last argument.


Reply to this email directly or view it on GitHub.

@jucor
Copy link
Contributor Author

jucor commented Nov 18, 2013

Comment by d11 from Monday Nov 11, 2013 at 10:39 GMT


On second thoughts, the 'flag' option doesn't work so well if we want to add support for passing the precision matrix too.

@jucor
Copy link
Contributor Author

jucor commented Nov 18, 2013

Comment by jucor from Monday Nov 11, 2013 at 10:39 GMT


I was thinking a hybrid of both:

multivariateGaussianRand( [n | resultTensor,] mu, covOrCholesky [, options])

where options is a table with

  • cholesky = true or false (default false) indicates whether we passed a cholesky or a variance
  • and any other setting that we may see fit to extend in the future, e.g. returnCholesky

@jucor
Copy link
Contributor Author

jucor commented Nov 18, 2013

Comment by d11 from Monday Nov 11, 2013 at 10:40 GMT


I see - yes, that could work quite well I think.

On 11 November 2013 10:39, Julien Cornebise notifications@github.comwrote:

I was thinking a hybrid of both:

multivariateGaussianRand( [n | resultTensor,] mu, covOrCholesky [, options])

where options is a table with

  • cholesky = true or false (default false) indicates whether we passed
    a cholesky or a variance
  • and any other setting that we may see fit to extend in the future,
    e.g. returnCholesky


Reply to this email directly or view it on GitHubhttps://github.com/google-deepmind/torch-randomkit/issues/5#issuecomment-28188896
.

@jucor
Copy link
Contributor Author

jucor commented Nov 18, 2013

Comment by jucor from Monday Nov 11, 2013 at 10:41 GMT


Yes, exactly: so we can have

multivariateGaussianRand( [n | resultTensor,] mu, covOrCholeskyOrPrecisionOrCholprecision [, options])

where options would have a flag for cholesky and a flag for precision -- thus allowing to pass the inverse of the Cholesky (although precision and cholesky-precision is more useful for logpdf rather than for the sampling, but at least we'd have a unified interface).

@jucor
Copy link
Contributor Author

jucor commented Nov 18, 2013

@d11 : done :) But I discovered #10 while doing this one...

jucor pushed a commit that referenced this pull request Nov 18, 2013
Pass Cholesky to multivariate gaussian
@jucor jucor merged commit c168982 into master Nov 18, 2013
@jucor jucor deleted the cholesky branch November 18, 2013 17:59
@d11
Copy link
Contributor

d11 commented Nov 18, 2013

Nice! Thanks! We should put a note in the docs, if you didn't already

@jucor
Copy link
Contributor Author

jucor commented Nov 18, 2013

I'm doing it right now ;-)

On 18 Nov 2013, at 18:05, Dan Horgan notifications@github.com wrote:

Nice! Thanks! We should put a note in the docs, if you didn't already


Reply to this email directly or view it on GitHub.

@jucor
Copy link
Contributor Author

jucor commented Nov 18, 2013

And done: http://jucor.github.io/torch-distributions/#toc_13

On 18 Nov 2013, at 18:05, Julien Cornebise julien@deepmind.com wrote:

I'm doing it right now ;-)

On 18 Nov 2013, at 18:05, Dan Horgan notifications@github.com wrote:

Nice! Thanks! We should put a note in the docs, if you didn't already


Reply to this email directly or view it on GitHub.

@d11
Copy link
Contributor

d11 commented Nov 18, 2013

Awesome :)
On 18 Nov 2013, at 18:09, Julien Cornebise notifications@github.com wrote:

And done: http://jucor.github.io/torch-distributions/#toc_13

On 18 Nov 2013, at 18:05, Julien Cornebise julien@deepmind.com wrote:

I'm doing it right now ;-)

On 18 Nov 2013, at 18:05, Dan Horgan notifications@github.com wrote:

Nice! Thanks! We should put a note in the docs, if you didn't already


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub.

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

Successfully merging this pull request may close these issues.

None yet

2 participants