Dev/fix latest numpy types #403
Conversation
| @@ -247,10 +249,12 @@ def score(self, X: np.ndarray, batch_size: int = int(1e10), return_predictions: | |||
| # model predictions | |||
| y = predict_batch(X, self.model, batch_size=batch_size) | |||
| y_recon = predict_batch(X_recon, self.model, batch_size=batch_size) | |||
| y = cast(np.ndarray, y) # help mypy out | |||
There was a problem hiding this comment.
Is there a specific reason not to use a type annotation like below here? Or is it just a convention we are going with?
y: np.ndarray = predict_batch(X, self.model, batch_size=batch_size)
There was a problem hiding this comment.
It doesn't work because y is already defined and --allow-redefinitions is not enabled.
There was a problem hiding this comment.
Ah that makes sense, thanks!
| @@ -83,16 +83,20 @@ def __init__( | |||
| else: | |||
| self.device = torch.device('cpu') | |||
|
|
|||
| # TODO: TBD: the several type:ignore's below are because x_ref is typed as an np.ndarray | |||
There was a problem hiding this comment.
Do you think --allow-redefinitions would cause mypy to miss anything significant elsewhere? I wonder if it's best to activate it now as redefining variable types is pretty common for us?
Otherwise, we could go with a convention of renaming variables instead, i.e. something like
x_ref_torch = cast(torch.Tensor, torch.as_tensor(self.x_ref).to(self.device)
Although the latter seems undesirable to me...
There was a problem hiding this comment.
Renaming wouldn't need the cast at all I think.
I'm a bit wary of enabling --allow-redefinitions as it's unclear as you say what we could potentially miss. Even so, it is quite limited as redefinitions would be allowed only within the same scope.
There was a problem hiding this comment.
Sorry yes cast is making more sense to me now!
I'd have thought that within the same scope would mostly be fine for us? But in any case, it's not clear to me what "within the same block and nesting depth" in the mypy docs means. Also, I take your point that it's probably worth being cautious about changing this.
There was a problem hiding this comment.
Well I don't know what happened but I just removed all these ignore's and locally the mypy check passes now...
There was a problem hiding this comment.
Nevermind, I was being silly as had downgraded back to numpy 1.19...
| @@ -20,6 +20,7 @@ exclude = | |||
| [mypy] | |||
| ignore_missing_imports = True | |||
| strict_optional = False | |||
| show_error_codes = True | |||
| @@ -245,7 +245,8 @@ def logp(self, dist, X: np.ndarray, return_per_feature: bool = False, batch_size | |||
| Log probabilities. | |||
| """ | |||
| logp_fn = partial(dist.log_prob, return_per_feature=return_per_feature) | |||
| return predict_batch(X, logp_fn, batch_size=batch_size) | |||
| # TODO: TBD: can this be any of the other types from predict_batch? i.e. tf.Tensor or tuple | |||
There was a problem hiding this comment.
I'm not sure about this one. I'd have guessed that predict_batch won't return tf.Tensor here since we don't explicitly set dtype (so return_np=True), but it looks like tuple might be possible? Hopefully @arnaudvl has more input!
|
There are a few more issues with the recently dropped |
This PR is based on the latest
numpywhich has type information. This flags a few issues with our types which have been resolved in this PR. It is a pre-requisite for upgrading totensorflow2.7.Most of this PR is pretty straight-forward, however I've left
TODO: TBDnotes where I would welcome a discussion, this is mostly to do with implicit narrowing of types that eithermypycan't handle (fixed easily with appropriatetype: ignore[code]) or more seriously with our logic where we haven't considered that a type might not be narrowed down sufficiently (e.g. is the userlistalways annp.ndarrayby the time it hits a function that only works onnp.ndarray?).There will be another PR enabling strict optional typing which will likely reveal a few more issues with current typing.