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

Use to_numpy on TensorType #477

Closed
wants to merge 1 commit into from

Conversation

uri-granta
Copy link
Collaborator

As noted in #347, we currently don't properly check numpy-related types since numpy 1.19 (required by TensorFlow 2.4 and 2.5) doesn't yet have type stubs . Looking at the typing issues caught in numpy 1.20, it seems most are the result of us using a TensorType result from e.g. a sample call as if it must be a tf.Tensor (which it will be in practice, but is not what the type says). We have two options on how to fix this:

  1. Continue to return TensorType but call trieste.utils.to_numpy rather than calling .numpy()
  2. Continue to accept TensorType for our inputs but only return tf.Tensors.

This PR shows what option 1 looks like in the code and in one of the notebooks. If it looks ok then I'll make it for the other notebooks too.

@hstojic
Copy link
Collaborator

hstojic commented Jan 25, 2022

why do we actually need anything other than tf.Tensors?

Copy link
Collaborator

@hstojic hstojic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have thought we have more of these?!
(we have some more in plotting utils in notebook folder...)

@uri-granta uri-granta marked this pull request as draft January 25, 2022 20:18
@uri-granta
Copy link
Collaborator Author

why do we actually need anything other than tf.Tensors?

Good question. I'll see if I can get the type checker to tell me which interfaces (if any) we're actually using with ndarrays rather than tf.Tensors. Also worth seeing what the others think.

I would have thought we have more of these?!

Yes, there are lots more. I just want to get feedback before I make more changes!

@hstojic
Copy link
Collaborator

hstojic commented Jan 25, 2022

why do we actually need anything other than tf.Tensors?

Good question. I'll see if I can get the type checker to tell me which interfaces (if any) we're actually using with ndarrays rather than tf.Tensors. Also worth seeing what the others think.

mostly we use TensorType with data, in these cases everything has to pass through Dataset so if we convert everything to a Tensor there we should be fine with tf.Tensor - one other usage that we probably have is as parameters/arguments, there it might be useful to allow both numpy and tensors

I would have thought we have more of these?!

Yes, there are lots more. I just want to get feedback before I make more changes!

Ok

Overall, to me switching to tf.Tensor might be the best solution, and after that your 2)

@uri-granta uri-granta closed this Jan 26, 2022
@uri-granta uri-granta deleted the uri/mypy_related_typing_errors branch April 19, 2022 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants