-
Notifications
You must be signed in to change notification settings - Fork 101
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
GPLVM tutorial #122
GPLVM tutorial #122
Conversation
α ~ MvLogNormal(MvNormal(K, 1.0)) | ||
σ ~ MvLogNormal(MvNormal(D, 1.0)) | ||
# use filldist for Zygote compatibility | ||
Z ~ filldist(Normal(0., 1.), K, N) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following will fix it:
Z ~ filldist(Normal(0., 1.), K, N) | |
Z_vec ~ filldist(Normal(0., 1.), K * N) | |
Z = reshape(Z_vec, K, N) |
It's due to the meanfield approximation constructed not handling higher-dim arrays properly. I have a PR open with a fix for this, but it never got any attention; I'll try to get that PR merged so the above workaround won't be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for the quick response!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am running into some sort of race condition/infinity loop now (or it's just taking realllly long). It's affecting both MCMC inference and ADVI. For ADVI I get an error message, whereas MCMC just seems to run forever. This happens when I uncomment these 2 lines, I added a reproducible example in the latest commit:
# using Zygote # Tracker supported? check it?
# Turing.setadbackend(:zygote)
Should I raise an issue for this in Turing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so -- I've been running into this on another project too, and honestly I'm kind of glad that someone else can replicate this issue outside of my project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably also what's timing out all the CIs 😕
EDIT: It actually seems like it's the GP in this case though. If you remove the Y ~ ...
it works.
Also, you probably don't want to use filldist(prior, D)
here; Y ~ prior
should do the trick (Turing just calls loglikelihood
under the hood, and so you can verify that this would work by just checking that loglikelihood(prior, Y)
returns a single float).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll propose a fix for the CI in a bit. It's some issue with the caching.
Could you make the name of the tutorial consistent with the other names? The pattern is
tutorials/02-logistic-regression/02_logistic-regression.jmd
[...]
tutorials/08-multinomial-logistic-regression/08_multinomial-logistic-regression.jmd
[...]
Also, it would be great if you can hide some assertions in the tutorial. That way, it's easier to check whether the tutorial also is built correctly in the future with different Julia versions or dependency versions.
Finally, maybe it's a good idea to split this PR up into two PRs. That should speed up the review process.
For example, the CI will fail when assertion blocks such as
fail. Note that this block is hidden from the output. This functionality is tested in https://github.com/TuringLang/TuringTutorials/blob/master/test/build.jl. |
@leachim The following error will be fixed if you merge master into your branch:
|
Many thanks for your help! I rebased this on master, hope that's okay now. Let me know if I did something wrong :) I'll add some assert later on, this is still a bit of work in progress, so bear with me for a little longer. I will let you know once it's ready for review again. |
Great 👍 Thanks. I see that the tests in CI are still broken, but that is my bad and not yours. Let me know when the tutorial works for you, then I'll fix the tests. |
Yeah, I'm afraid that something went wrong. PRs which add a new tutorial should only add three files. For example, to add a new tutorial "my-bayesian-tutorial", the PR should only add the files:
and, locally, the build should succeed when doing
I'm aware that this isn't super easy to do and could be easier. I'm working on that, but also have to do my 40 hours a week job and a book that I'm working on, so it will take a while still 🙂 |
It's probably easiest to just copy your files into a new and up-to-date branch and open a new PR. |
e98fc74
to
0fdac31
Compare
Okay, so I rebased this on master and I also squased some commits, so should be pretty much equivalent to opening a new pull request, but we can keep the discussion history. I am still working on the tutorial. There is two things that might cause issues at the moment
I'll try to speed it up more, but not sure how much I can reduce runtime. I am also still working on details and fine-tuning things. |
That's very unfortunate yes, because it makes maintaining the tutorial very difficult. If I look at the other tutorials, it would probably mean that it will be outdated/not working in a few months to a year. Can we find a solution for that? Maybe a smaller dataset or simpler example? |
update naming for creation refactor files
add dependencies
0fdac31
to
3debcec
Compare
From the logs:
The build will work locally because you have probably added |
...ials/12-gaussian-process-latent-variable-model/12_gaussian-process-latent-variable-model.jmd
Outdated
Show resolved
Hide resolved
|
||
Let's start by loading some dependencies. | ||
```julia | ||
# Load Turing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Load Turing. |
...ials/12-gaussian-process-latent-variable-model/12_gaussian-process-latent-variable-model.jmd
Show resolved
Hide resolved
StatsBase.transform!(dt, oil_matrix); | ||
``` | ||
|
||
We will start out, by demonstrating the basic similarity between pPCA (see the tutorial on this topic) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will start out, by demonstrating the basic similarity between pPCA (see the tutorial on this topic) | |
We will start out by demonstrating the basic similarity between pPCA (see the tutorial on this topic) |
# Priors | ||
α ~ MvLogNormal(MvNormal(K, 1.0)) | ||
σ ~ LogNormal(0.0, 1.0) | ||
Z ~ filldist(Normal(0., 1.), K, N) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Z ~ filldist(Normal(0., 1.), K, N) | |
Z ~ filldist(Normal(0.0, 1.0), K, N) |
More explicit/clear and also consistent with other uses in the tutorial.
prior = gp(ColVecs(Z), noise) | ||
|
||
for d in 1:D | ||
Y[:, d] ~ prior |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacing inconsistent. Here is a space (Y[:, d]
) and above it is not (x[:,d]
). There is no good reason to not stick to one style and use it everywhere. I would suggest using a space behind the comma. See also "Use whitespace to make the code more readable." at BlueStyle.
chain_ppca = sample(ppca, NUTS(), 1000) | ||
``` | ||
```julia | ||
w = permutedims(reshape(mean(group(chain_ppca, :w))[:,2], (n_features,n_features))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can a simple one liner function be defined for these two? The name of the one-liner could also shed light on what these lines do exactly.
Also applies to the 4 or 5 occurrences of this kind of sentence below.
``` | ||
|
||
```julia | ||
df_gplvm_linear = DataFrame(z_mean', :auto) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a small clarification for this block?
|
||
### Speeding up inference | ||
|
||
Gaussian processes tend to be slow, as they naively require. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end of sentence missing
...ials/12-gaussian-process-latent-variable-model/12_gaussian-process-latent-variable-model.jmd
Outdated
Show resolved
Hide resolved
@leachim, you got the "ERROR: LoadError: LoadError: TypeError: in Type{...} expression, expected UnionAll, got Type{Parsers.Options}" error (more info in JuliaData/CSV.jl#862). To fix it, simply update the CSV dependency to 0.9. |
...ials/12-gaussian-process-latent-variable-model/12_gaussian-process-latent-variable-model.jmd
Outdated
Show resolved
Hide resolved
...ials/12-gaussian-process-latent-variable-model/12_gaussian-process-latent-variable-model.jmd
Outdated
Show resolved
Hide resolved
...ials/12-gaussian-process-latent-variable-model/12_gaussian-process-latent-variable-model.jmd
Outdated
Show resolved
Hide resolved
...ials/12-gaussian-process-latent-variable-model/12_gaussian-process-latent-variable-model.jmd
Outdated
Show resolved
Hide resolved
...ials/12-gaussian-process-latent-variable-model/12_gaussian-process-latent-variable-model.jmd
Outdated
Show resolved
Hide resolved
...ials/12-gaussian-process-latent-variable-model/12_gaussian-process-latent-variable-model.jmd
Outdated
Show resolved
Hide resolved
using AbstractGPs, KernelFunctions, Random, Plots | ||
|
||
using Distributions, LinearAlgebra | ||
using VegaLite, DataFrames, StatsPlots, StatsBase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason for using both StatsPlots and VegaLite? Seems a bit complicated to use two different plotting frameworks in one tutorial.
...ials/12-gaussian-process-latent-variable-model/12_gaussian-process-latent-variable-model.jmd
Outdated
Show resolved
Hide resolved
...ials/12-gaussian-process-latent-variable-model/12_gaussian-process-latent-variable-model.jmd
Outdated
Show resolved
Hide resolved
Z ~ filldist(Normal(0.0, 1.0), K, N) | ||
mu ~ filldist(Normal(0.0, 1.0), N) | ||
|
||
kernel = σ * SqExponentialKernel() ∘ ARDTransform(α) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the function from above?
Yes, thanks. There is an open compat helper pull request in RDatasets, so once that is merged, I will update the version of CSV. Currently, RDatasets only supports up to 0.8. |
Co-authored-by: David Widmann <[email protected]> Co-authored-by: Rik Huijzer <[email protected]>
This is some initial code for the GPLVM model. @torfjelde I am trying to get ADVI working for this example, but there is an error popping up to do with logabsdetjac. Do you mind having a look at this, since you seem to have worked on the bijectors package before.
It is working with the NUTS sampler inference, but running the same model with ADVI throws this error.