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

update so greta.gp works with greta 0.3.0.x #9

Open
wants to merge 26 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jdyen
Copy link

commented Mar 22, 2019

  • added tf_kernels.R to replace gpflow kernels used previously.
  • updated existing tests and added tests for kernels when inputs differ in numbers of rows.
  • added tf_slice function to allow active_dims on kernels.
  • kernels are tested but defns are not confirmed correct for all kernels.
  • minor updates to docs (e.g. removed gpflow requirements, adding v 0.3.0 of greta) but no major changes.
@goldingn
Copy link
Member

left a comment

Nice one, this is great!

I can't tell from the code whether this accounts for the batch dimension greta now adds to all variables, and uses for MCMC. I haven't been able to test it yet, because of the as.greta_array() issue, but I can take a look and double check once that's fixed.

DESCRIPTION Outdated
@@ -2,20 +2,19 @@ Package: greta.gp
Type: Package
Title: Gaussian Process Modelling in greta
Version: 0.1.3

This comment has been minimized.

Copy link
@goldingn

goldingn Mar 22, 2019

Member

could you please bump to 0.1.4?

This comment has been minimized.

Copy link
@jdyen

jdyen Mar 22, 2019

Author

done

DESCRIPTION Outdated
@@ -2,20 +2,19 @@ Package: greta.gp
Type: Package
Title: Gaussian Process Modelling in greta
Version: 0.1.3
Date: 2017-09-09
Date: 2019-02-21
Author: Nick Golding

This comment has been minimized.

Copy link
@goldingn

goldingn Mar 22, 2019

Member

please add your self to the authorship!
Code example here: https://github.com/greta-dev/greta/blob/master/DESCRIPTION#L6-L21

This comment has been minimized.

Copy link
@jdyen

jdyen Mar 22, 2019

Author

done. added as ctb and updated to use the suggested format

NAMESPACE Outdated
export(rbf)
export(white)

This comment has been minimized.

Copy link
@goldingn

goldingn Mar 22, 2019

Member

did you mean to de-export the white kernel?

This comment has been minimized.

Copy link
@jdyen

jdyen Mar 22, 2019

Author

I did because I didn't understand why it was returning all zeros in some cases. All clear now, added back in.

R/package.R Outdated
#' @importFrom greta .internals
#'
NULL

This comment has been minimized.

Copy link
@goldingn

goldingn Mar 22, 2019

Member

The package is erroring for me when running the examples. E.g.:

library(greta.gp)
k1 <- rbf(lengthscales = c(0.1, 0.2), variance = 0.6)
Error in lapply(parameters, as.greta_array) : 
  object 'as.greta_array' not found 

I think that's because you don't get the unexported function as.greta_array() from greta. You can do that with:

as.greta_array <- greta::.internals$greta_arrays$as.greta_array

and here is a good place to put it. you may need to do that for other internal functions too

This comment has been minimized.

Copy link
@jdyen

jdyen Mar 22, 2019

Author

have added as.greta_array and op to package.R. Not sure if op is needed.

gpflow_name = "White",
parameters = list(variance = variance),
dim = dim)
constant <- function (variance) {

This comment has been minimized.

Copy link
@goldingn

goldingn Mar 22, 2019

Member

I see you've got both bias and constant doing the same thing. Was that deliberate? If so, we should mention in the docs that bias and constant are synonymous.

This comment has been minimized.

Copy link
@jdyen

jdyen Mar 22, 2019

Author

Added a note to the docs. Copied the use of both from gpflow.

Show resolved Hide resolved R/kernel_constructors.R
@goldingn

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

goddamned pip.

Here's the greta travis installation code you probably need: https://github.com/greta-dev/greta/blob/master/.travis.yml#L74-L78

@jdyen

This comment has been minimized.

Copy link
Author

commented Mar 22, 2019

Should be working with batch dims from greta but I haven't tested this explicitly. Examples run > 1 chain without error. This doesn't test whether it's calculating correctly but suggests that batch dims aren't causing issues in any of the tensor calcs.

@jdyen

This comment has been minimized.

Copy link
Author

commented Mar 22, 2019

Tried a few things but can't get tf and tfp installed on travis properly

@goldingn

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

Annoying. I'll see if I can find a way around it.

@goldingn

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Running the checks locally, I get failures in test_gps.R:

Failure: gaussian processes can be defined in models and sampled from
Failure: gaussian processes can be projected to new data
Failure: gaussian processes can be projected with a different kernel

When running the example in the readme I get the following error (on running model(f_plot)):

 Error in tf$gather(X, dims, axis = -1L) : attempt to apply non-function 

There are also some = to switch to <- in the readme

@goldingn

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

I have no earthly idea why travis is seeing TF and TFP for the greta repo, but not for this one. I've tried clearing caches a couple of times, and can't see any non-trivial differences in the travis yamls. Will think on it.

Can you replicate those test failures I'm seeing on your machine?

@jdyen

This comment has been minimized.

Copy link
Author

commented Mar 28, 2019

Can't replicate any of those errors but have updated the readme and tried one change to the tfp installation in the travis yml.

@jdyen

This comment has been minimized.

Copy link
Author

commented Mar 28, 2019

I tried to recreate those errors on two computers using greta 0.3.0.9001 and tf 1.12 and tfp 0.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.