Migrate MCMC notebook to gpflow2.0 #1100

cdmatters · 2019-10-14T14:15:31Z

Migrate MCMC notebook to gpflow 2 using tensorflow probabilities.
Changes required a few changes across the rest of the codebase, outside of the notebook.

small change to leaf printing function (fix bug, which was preventing printing of composite kernels)
parameters and trainable_parameters now return tuples, not generators, like tf implementation of variables, trainable_variables
tfp.distributions now work with different dtypes (by wrapping of parameters), so now play nicely with gpflow.

Other changes required including: * small change to leaf printing function (fix bug, preventing printing of composite kernels) * `parameters` and `trainable_parameters` now return tuples, not generators, like tf implementation of `variables`, `trainable_variables` * tfp.distributions now have a choice of dtype (by wrapping of parameters)

codecov · 2019-10-14T14:16:03Z

Codecov Report

Merging #1100 into develop will decrease coverage by <.01%.
The diff coverage is 95.12%.

@@             Coverage Diff             @@
##           develop    #1100      +/-   ##
===========================================
- Coverage    95.49%   95.48%   -0.01%     
===========================================
  Files           67       68       +1     
  Lines         3084     3125      +41     
===========================================
+ Hits          2945     2984      +39     
- Misses         139      141       +2

Impacted Files	Coverage Δ
gpflow/optimizers/__init__.py	`100% <100%> (ø)`	⬆️
gpflow/optimizers/mcmc.py	`95% <95%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff339f8...0db2ea0. Read the comment docs.

…lop-2.0/hmc-helper

gpflow/utilities/utilities.py

st-- · 2019-10-14T14:42:04Z

doc/source/notebooks/advanced/multiclass_classification.py

+    Fpred, Ypred = [], []
+    num_samples = len(samples[0])
+    for i in range(burn, num_samples, thin):
+        hparams = [hp[i] for hp in samples]


what does hp stand for?

st-- · 2019-10-14T14:42:36Z

doc/source/notebooks/advanced/multiclass_classification.py

+    num_samples = len(samples[0])
+    for i in range(burn, num_samples, thin):
+        hparams = [hp[i] for hp in samples]
+        [var.assign(hp) for var, hp in zip(m.trainable_variables, hparams)]


How does this compare in speed to the tf1 with feed-dict?

no clue. Theres clearly iteration overhead to consider as well as the unknown speed of assign. I dont think we have a choice either way, though worth investigating

Also, in general, I think this list comp is quite ugly and there should be a different way to set all the variables of the model, which is cleaner.

Comparing them would be wrong as they are different things

@condnsdmatters , there is a utility function for multiple assign

st-- · 2019-10-14T14:44:01Z

gpflow/likelihoods/robustmax.py

@@ -22,7 +23,8 @@ class RobustMax(tf.Module):
    def __init__(self, num_classes, epsilon=1e-3, **kwargs):
        super().__init__(**kwargs)
        transform = tfp.bijectors.Sigmoid()
-        prior = tfp.distributions.Beta(0.2, 5.)
+        fdtype = lambda x: np.array(x, dtype=default_float())


what does the f stand for?
if this is something needed throughout the code base, would it be worthwhile putting it into gpflow.utilities or so ?

f here stands for float. i think this is a reasonable thing to go to utilities, perhaps under the name def to_default_float(x) etc

gpflow/optimizers/mcmc.py

st-- · 2019-10-14T14:55:37Z

gpflow/optimizers/mcmc.py

+                sample = param.transform.forward(values)
+            else:
+                sample = values
+            samples.append(sample.numpy())


Is the casting to numpy needed? Would this slow down the execution?

gpflow/optimizers/mcmc.py

st-- · 2019-10-14T14:56:49Z

gpflow/optimizers/mcmc.py

+        parameters: List of `Variable`'s or gpflow `Parameter`s used as a state of the Markov chain.
+    """
+
+    target_log_prob_fn: Callable[[ModelParameters], tf.Tensor]


Actually, this is used as if it doesn't take any arguments, so maybe the type hint is wrong?

gpflow/optimizers/mcmc.py

awav · 2019-10-14T20:24:30Z

@condnsdmatters, Thanks for updating the notebook.

Could we not mix thing up in a single PR?

small change to leaf printing function (fix bug, which was preventing printing of composite kernels)

That must be separate PR with testing and cetera. I have a suspicion that it has been resolved already.

parameters and trainable_parameters now return tuples, not generators, like tf implementation of variables, trainable_variables.

I don't think that variables return a tuple, it is a list. Why this change is important here?

tfp.distributions now work with different dtypes (by wrapping of parameters), so now play nicely with gpflow.

I don't understand the problem with it. Could you explain what you are trying to do with it?

In general, when I wrote SamplingHelper, I adjusted it to the specific project for solving the needs. It requires much more thought about how the interface should look for a user. Let's discuss it offline.

Thanks!

cdmatters · 2019-10-15T15:12:58Z

In reply to @awav, I think I misunderstood the helper you wrote - my understanding was I should use as is, rather than thinking about interfaces, etc. I'm very happy to discuss and change, I agree it seems to be a bit of an odd class atm.

Vis a vis other changes:

The printing does indeed seem to be fixed
The variables and trainable_variables parameters on tf.Module return tuples technically not lists: see here However, the key problem here is that at the moment the module wrapper we have neither returns lists nor tuples, but rather a generator for self._flatten. If you don't wrap the generator in an iterable, when you pass the generator to the frozenset SampleHelper, the generator exhausts after its first use. eg:
```
params = m.trainable_parameters
y = [p for p in params]  # List[Params]
z = [p for p in params]  # [] Empty! 
```
The problem with tf.distributions is that there is no way to chose the dtype explicitly for the Normal and Gamma distributions, and there is no way to change the default dtype in tensorflow (as per the issue you opened). tfp implementation of these distributions instead infers the dtype from the dtype of the parameters to the distribution eg here, and then passes these explicitly to the base Distribution class. The end result is that if you want to chose the dtype for the distributions (as we need to when operating in 64bit mode in gpflow), the only way you can do this is by wrapping the parameters before sending them in to the constructor. Most unsatisfactory I know.

I am more than happy to put these into separate commits and PRs, as I do agree that's better from a git commit history point of view. I can restructure these now.

@st-- I'll also address all the comments in this PR before doing that.

Co-Authored-By: st-- <st--@users.noreply.github.com>

awav · 2019-11-03T23:18:49Z

Hello @condnsdmatters , I apologize for closing your PR. It was closed automatically when I moved develop-2.0 to the develop. Could you re-open the PR? Thanks!

doc/source/notebooks/advanced/multiclass_classification.py

gpflow/optimizers/mcmc.py

awav · 2019-11-11T14:40:58Z

gpflow/optimizers/mcmc.py

+
+        return _target_log_prob_fn_closure
+
+    def convert_samples_to_parameter_values(self, hmc_samples):


Suggested change

def convert_samples_to_parameter_values(self, hmc_samples):

def convert_to_constrained_values(self, hmc_samples):

Co-Authored-By: Artem Artemev <art.art.v@gmail.com>

cdmatters · 2019-11-13T10:28:44Z

@awav I fundamentally cant change the grad function to remove None's due to the requirements that we differentiate w.r.t to the held params on the model. As it says in the tf.custom_gradient documentation:

If f uses Variables (that are not part of the inputs), i.e. through get_variable, then grad_fn should have signature g(*grad_ys, variables=None), where variables is a list of the Variables, and return a 2-tuple (grad_xs, grad_vars), where grad_xs is the same as above, and grad_vars is a list with the derivatives of Tensors in y with respect to the variables.

gpflow/optimizers/mcmc.py

…w into eric/develop-2.0/hmc-helper

awav and others added 3 commits October 7, 2019 09:37

MCMC helper

8910b50

Update docstring of the helper

36c1a01

Eric Hambro added 2 commits October 14, 2019 15:20

Committing a full run, with all plots

c717d60

Merge branch 'develop-2.0' of github.com:GPflow/GPflow into eric/deve…

f622923

…lop-2.0/hmc-helper

cdmatters requested a review from awav October 14, 2019 14:39

st-- reviewed Oct 14, 2019

View reviewed changes

remove item from blacklist

63a0cdf

Apply suggestions from code review

f24528e

Co-Authored-By: st-- <st--@users.noreply.github.com>

This was referenced Oct 16, 2019

gpflow.Module.parameter (base for Model) returns an tuple, not generator #1102

Merged

Ensure all tf_probability distributions take the correct dtype parameters #1103

Merged

st-- and others added 7 commits October 23, 2019 00:26

Merge branch 'develop-2.0' into eric/develop-2.0/hmc-helper

607d652

Merge branch 'develop-2.0' into eric/develop-2.0/hmc-helper

9a6ca3f

remove whitespace changes

32d45df

save

2866e2c

just need to update docstring

e8acd20

fixup

1984b79

Merge branch 'develop-2.0' into eric/develop-2.0/hmc-helper

1aee905

awav closed this Nov 3, 2019

awav reopened this Nov 3, 2019

awav changed the base branch from develop-2.0 to develop November 3, 2019 23:30

awav reviewed Nov 11, 2019

View reviewed changes

doc/source/notebooks/advanced/multiclass_classification.py Show resolved Hide resolved

awav reviewed Nov 11, 2019

View reviewed changes

gpflow/optimizers/mcmc.py Outdated Show resolved Hide resolved

awav reviewed Nov 11, 2019

View reviewed changes

gpflow/optimizers/mcmc.py Outdated Show resolved Hide resolved

awav requested changes Nov 11, 2019

View reviewed changes

Merge branch 'develop' into eric/develop-2.0/hmc-helper

1631868

Eric Hambro and others added 3 commits November 12, 2019 16:09

fixup changes from Artem

196467f

Update gpflow/optimizers/mcmc.py

00bc7b7

Co-Authored-By: Artem Artemev <art.art.v@gmail.com>

fixing rename of function

fc04378

Apply suggestions from code review

231ff0d

awav reviewed Nov 13, 2019

View reviewed changes

gpflow/optimizers/mcmc.py Outdated Show resolved Hide resolved

awav and others added 5 commits November 13, 2019 10:38

Update gpflow/optimizers/mcmc.py

a393f3f

Merge branch 'eric/develop-2.0/hmc-helper' of github.com:GPflow/GPflo…

35f2073

…w into eric/develop-2.0/hmc-helper

added tests

e2fc15d

make tests run faster with fewer sample steps

b432a4f

test the wrapped closure

0db2ea0

awav approved these changes Nov 13, 2019

View reviewed changes

awav merged commit 24b5733 into develop Nov 13, 2019

awav deleted the eric/develop-2.0/hmc-helper branch November 13, 2019 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate MCMC notebook to gpflow2.0 #1100

Migrate MCMC notebook to gpflow2.0 #1100


		return _target_log_prob_fn_closure

		def convert_samples_to_parameter_values(self, hmc_samples):

	def convert_samples_to_parameter_values(self, hmc_samples):
	def convert_to_constrained_values(self, hmc_samples):

Migrate MCMC notebook to gpflow2.0 #1100

Migrate MCMC notebook to gpflow2.0 #1100

Conversation

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment