predict_f and predict_y as NamedTuple#1657
Conversation
st--
left a comment
There was a problem hiding this comment.
Thanks for the contribution - I think this will be really helpful! I'd just like to hear from some of the other people involved with the GPflow project what their thoughts on the naming of the variance field are. :)
|
|
||
|
|
||
| # %% [markdown] | ||
| # Moreover, you can get the mean and variance data individually for example as `m.predict_f(xx).mean` instead of `m.predict_f(xx)[0]`. |
There was a problem hiding this comment.
| # Moreover, you can get the mean and variance data individually for example as `m.predict_f(xx).mean` instead of `m.predict_f(xx)[0]`. | |
| # Moreover, you can get the mean and variance predictions individually, using for `m.predict_f(xx).mean` instead of `m.predict_f(xx)[0]` and `m.predict_f(xx).variance` instead of `m.predict_f(xx)[1]`. |
(note- if we rename the variance field we'll have to update this)
| MeanAndVariance = Tuple[tf.Tensor, tf.Tensor] | ||
|
|
||
| class MeanAndVariance(NamedTuple): | ||
| """ NamedTuple to access mean- and variance-function separately """ |
There was a problem hiding this comment.
| """ NamedTuple to access mean- and variance-function separately """ | |
| """ NamedTuple that holds mean and variance as named fields """ |
| """ NamedTuple to access mean- and variance-function separately """ | ||
|
|
||
| mean: tf.Tensor | ||
| variance: tf.Tensor |
There was a problem hiding this comment.
More of a general question at other maintainers (@vdutor @markvdw @awav) & anyone else ...: what should the name of this field be? In code we often abbreviate the variance to "var", but also it sometimes represents the covariance... or maybe that should be a different type? MeanAndVariance and MeanAndCov (and return a different one depending on full_cov etc)?
There was a problem hiding this comment.
i like the idea of the two types. Haven't thought about it in detail
There was a problem hiding this comment.
do you always know for sure if you've got the cov or var?
There was a problem hiding this comment.
If you pass in full_cov=False and full_output_cov=False, you get the marginals back. If one of full_cov or full_output_cov is True, you get the covariance over inputs or outputs, respectively. If both are True, you should get the N P x N P covariance matrix (though this combination isn't actually implemented in several cases, I believe). So the output type is solely determined by the full_cov and full_output_cov arguments.
There was a problem hiding this comment.
btw you can use typing.overload and typing.Literal to give more information than just MeanAndVariance | MeanAndCov, if you wanted to do that
There was a problem hiding this comment.
I've created a small example, which should work as wanted:


But I' not comfortable with the if-else statement :/
from typing import Literal, NamedTuple, overload
class MeanAndVariance(NamedTuple):
mean: int
variance: int
class MeanAndCovariance(NamedTuple):
mean: int
covariance: int
@overload
def predict_f(auto_cov: Literal[False]) -> MeanAndVariance:
...
@overload
def predict_f(auto_cov: Literal[True]) -> MeanAndCovariance:
...
def predict_f(auto_cov: bool = False) -> (MeanAndVariance | MeanAndCovariance):
# calculations
return MeanAndCovariance(1, 2) if auto_cov else MeanAndVariance(1, 2)There was a problem hiding this comment.
I don't think there's a good way around the if-else, and I think it's better to be explicit than the ambiguity of having to remember whether it's a [N, Q] or [Q, N, N] tensor ...:) I'd be happy with this.
There was a problem hiding this comment.
As it pointed out, typing.Literal is only available from Python 3.8 and up :/
There was a problem hiding this comment.
@antonykamp the typing_extensions module provides backports for older versions of Python, it does seem to include Literal. :)
There was a problem hiding this comment.
I added this pattern with overload and Tuple to the abstract method predict_f in gpflow/models/models.py. Should the overloads of predict_f be marked as abstract too? In any case I see no chance to test the Ellipsis operator of each overloaded funciton :/
Also, I wanted to ask if the parameters of the model constructor should be listed in the parametrization?
|
Hi @antonykamp could you make sure to update RELEASE.md and CONTRIBUTORS.md as part of this PR? (See #1660 and #1661). |
I'll take care of it after the "literal-overload-naming"-question is resolved. Otherwise, I would have to change it multiple times probably :) |
|
Closed because it's old :) |
PR type: enhancement
Related issue: #1567
Summary
Proposed changes
predict_forpredict_y, GPflow returns an instance ofMeanAndVariance, a subclass of NamedTuple. By that, the user can access mean and variance individually.What alternatives have you considered?
I've considered shorter names for
.meanand.variancebut rejected this idea because the current resulting code is a) more readable b) already shorter than before.Minimal working example
PR checklist
make format)make check-all)Release notes
Fully backwards compatible: yes
Commit message (for release notes):
(Is likely to be rebased)