Sc336/categorical kernel #2055
Sc336/categorical kernel #2055
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #2055 +/- ##
===========================================
+ Coverage 98.01% 98.06% +0.04%
===========================================
Files 96 97 +1
Lines 5404 5436 +32
===========================================
+ Hits 5297 5331 +34
+ Misses 107 105 -2
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
uri-granta
left a comment
There was a problem hiding this comment.
Various comments! Also probably worth checking with victor regarding which essential tests are worth adding. Code looks good though!
gpflow/kernels/categorical.py
Outdated
| from ..base import Parameter, TensorType | ||
| from . import Kernel | ||
|
|
||
| tfd = tfp.distributions |
There was a problem hiding this comment.
More normal to write
| tfd = tfp.distributions | |
| from tensorflow_probability import distributions as tfd |
gpflow/kernels/categorical.py
Outdated
| from . import Kernel | ||
|
|
||
| tfd = tfp.distributions | ||
| st = tfp.bijectors.Softplus().inverse |
There was a problem hiding this comment.
I don't think you're actually using st or tfd!
| st = tfp.bijectors.Softplus().inverse |
gpflow/kernels/categorical.py
Outdated
| of degrees of freedom in optimisation. You may also find fixing the lengthscale to be useful here. | ||
| Note that Z is parameterised by the differences of latent space values for each category for the same reason. | ||
| Multiple categories can be included by wrapping multiple layers of CategoricalKernel. | ||
| :param wrapped_kernel_1: The non-categorical kernel. |
There was a problem hiding this comment.
Surely clearer to call these arguments non_categorical_kernel and categorical_kernel?
| # wrapped_kernel_2 is the one that trains the categorical variables | ||
| set_trainable(wrapped_kernel_2, False) | ||
| self.wrapped_kernel = wrapped_kernel_1 * wrapped_kernel_2 | ||
| label_dim = 1 |
There was a problem hiding this comment.
I thought the categorical_kernel was supposed to specify the label dimension in its active_dims?
There was a problem hiding this comment.
We only support one label dimension for now, this could be extended at a later date.
| set_trainable(wrapped_kernel_2, False) | ||
| self.wrapped_kernel = wrapped_kernel_1 * wrapped_kernel_2 | ||
| label_dim = 1 | ||
| self._Z_deltas = Parameter( |
There was a problem hiding this comment.
A comment might be useful?
# parametrise by the `num_labels - 1` differences of latent space values
| """ | ||
| Z = tf.concat([tf.constant(0, shape=(1,), dtype=tf.float64), tf.squeeze(self._Z_deltas)], 0) | ||
| m = tf.linalg.band_part(tf.ones([tf.size(Z), tf.size(Z)], dtype=tf.float64), -1, 0) | ||
| return tf.expand_dims(tf.linalg.matvec(m, Z), -1) |
There was a problem hiding this comment.
looks like voodoo to me but I believe you! (though a simple unit test might be useful)
There was a problem hiding this comment.
I'll add a comment to explain it.
| np.testing.assert_allclose(K, K1 + K2) | ||
|
|
||
|
|
||
| def test_concat_inputs_with_latents() -> None: |
There was a problem hiding this comment.
I feel like there are definitely missing tests for the Categorical kernel (beyond broadcasting), but I'm probably not the person to spot them! Also don't we want a test that actually uses Categorical in a model?
c00be2e to
748352b
Compare
uri-granta
left a comment
There was a problem hiding this comment.
Looks good (though my OCD is still vexed by _concat_inputs_with_latents describing the same dimension as both D and x_dim!). Are you ok to create an issue to investigate writing more tests?
|
PR type: new feature
Summary
Proposed changes
Adds a new categorical kernel that implements categorical variables by mapping them to values in a latent space.
Minimal working example
# Put your example code in hereFully backwards compatible: yes
PR checklist
make format)make check-all)