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

GPLVM tutorial #122

Merged
merged 16 commits into from Mar 3, 2022
Merged

GPLVM tutorial #122

merged 16 commits into from Mar 3, 2022

Conversation

leachim
Copy link
Collaborator

@leachim leachim commented Jul 13, 2021

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.

α ~ MvLogNormal(MvNormal(K, 1.0))
σ ~ MvLogNormal(MvNormal(D, 1.0))
# use filldist for Zygote compatibility
Z ~ filldist(Normal(0., 1.), K, N)
Copy link
Member

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:

Suggested change
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.

Copy link
Collaborator Author

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!

Copy link
Collaborator Author

@leachim leachim Jul 16, 2021

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?

Copy link
Member

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.

Copy link
Member

@torfjelde torfjelde Jul 16, 2021

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@rikhuijzer rikhuijzer left a 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.

@rikhuijzer
Copy link
Contributor

For example, the CI will fail when assertion blocks such as

```julia; echo=false
@assert false       
```

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.

@rikhuijzer
Copy link
Contributor

@leachim The following error will be fixed if you merge master into your branch:

IOError: open("/home/runner/work/TuringTutorials/TuringTutorials/ClonedTuringTutorials/html/12-gaussian-process-latent-variable-model", 0, 0): no such file or directory (ENOENT)

@leachim
Copy link
Collaborator Author

leachim commented Aug 12, 2021

@leachim The following error will be fixed if you merge master into your branch:

IOError: open("/home/runner/work/TuringTutorials/TuringTutorials/ClonedTuringTutorials/html/12-gaussian-process-latent-variable-model", 0, 0): no such file or directory (ENOENT)

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.

@rikhuijzer
Copy link
Contributor

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.

@rikhuijzer
Copy link
Contributor

I rebased this on master, hope that's okay now. Let me know if I did something wrong :)

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:

tutorials/11-my-bayesian-tutorial/11_my-bayesian-tutorial.jmd
tutorials/11-my-bayesian-tutorial/Project.toml
tutorials/11-my-bayesian-tutorial/Manifest.toml

and, locally, the build should succeed when doing

julia> using TuringTutorials

julia> build("11-my-bayesian-tutorial");

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 🙂

@rikhuijzer
Copy link
Contributor

It's probably easiest to just copy your files into a new and up-to-date branch and open a new PR.

@leachim
Copy link
Collaborator Author

leachim commented Aug 19, 2021

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 have included two files with data (it's a small example that is commonly used in the literature, and so would be nice to have)
  • The run time is really long (more than 10hrs at the moment). I think this is similar to the 10: Bayesian differential equations tutorial.

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.

@rikhuijzer
Copy link
Contributor

The run time is really long (more than 10hrs at the moment). I think this is similar to the 10: Bayesian differential equations tutorial.

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?

@rikhuijzer
Copy link
Contributor

From the logs:

Error: ArgumentError: Package Turing not found in current path:
 - Run `import Pkg; Pkg.add("Turing")` to install the Turing package.

The build will work locally because you have probably added Turing.jl to your global environment, but not to the gplvm tutorial environment.


Let's start by loading some dependencies.
```julia
# Load Turing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Load Turing.

StatsBase.transform!(dt, oil_matrix);
```

We will start out, by demonstrating the basic similarity between pPCA (see the tutorial on this topic)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Contributor

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)))
Copy link
Contributor

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)
Copy link
Contributor

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end of sentence missing

@rikhuijzer
Copy link
Contributor

@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.

using AbstractGPs, KernelFunctions, Random, Plots

using Distributions, LinearAlgebra
using VegaLite, DataFrames, StatsPlots, StatsBase
Copy link
Member

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.

Z ~ filldist(Normal(0.0, 1.0), K, N)
mu ~ filldist(Normal(0.0, 1.0), N)

kernel = σ * SqExponentialKernel() ∘ ARDTransform(α)
Copy link
Member

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?

@leachim
Copy link
Collaborator Author

leachim commented Sep 13, 2021

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.
I managed to get the time requirement down to 2hrs or so, so it should be fine to include this now. Once I have the dependencies fixed, and I had another look, I will remove the draft status and then I would ask you to have another look at it.
Thanks again!

leachim and others added 3 commits September 13, 2021 14:39
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: Rik Huijzer <rikhuijzer@pm.me>
@yebai yebai marked this pull request as ready for review March 3, 2022 16:08
@yebai yebai merged commit e4635c0 into master Mar 3, 2022
@delete-merged-branch delete-merged-branch bot deleted the leachim/gplvm-tutorial branch March 3, 2022 16:09
@yebai yebai mentioned this pull request Mar 3, 2022
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

6 participants