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

What is correct behavior of keras.ops.eye? #20616

Open
yubori opened this issue Dec 10, 2024 · 5 comments · May be fixed by #20683
Open

What is correct behavior of keras.ops.eye? #20616

yubori opened this issue Dec 10, 2024 · 5 comments · May be fixed by #20683
Assignees
Labels
backend:tensorflow Good first issue Issues which are good for first-time contributors. Usually easy to fix. stat:contributions welcome A pull request to fix this issue would be welcome. type:Bug

Comments

@yubori
Copy link

yubori commented Dec 10, 2024

Hi,

I have a question about keras.ops.eye of Keras3.

With Tensorflow, the keras.ops.eye accepts integers and floats.
However, with torch and jax, only integers are accepted.

Which behavior is correct?

There is no limitation description on documents.
Because Keras 2 accepts integers and floats, I was confused with it during migration.

@dhantule
Copy link
Contributor

Hi @yubori,
thanks for reporting this. keras.ops.eye will return a 2-D tensor with ones on the diagonal and zeros elsewhere (identity matrix). However, you can control the data type of the resulting matrix to be float32 or float64. The values on the diagonal will still be 1.0 and the other elements will be 0.0 . Attaching gist.

@dhantule dhantule added type:Bug stat:awaiting response from contributor type:support User is asking for help / asking an implementation question. Stackoverflow would be better suited. and removed type:Bug labels Dec 10, 2024
@yubori
Copy link
Author

yubori commented Dec 10, 2024

@dhantule Sorry for the lack of my description.

This is a very simple issue.

Please see the following gist.

@dhantule
Copy link
Contributor

@yubori, Thanks for reporting this. I have reproduced this error here and keras.ops.eye produces error when using float values as input dimensions with Jax and PyTorch as backends. Although in Jax the jax.numpy.eye operation doesn’t support float values, similarly with torch.eye. We will look into this issue more. Thanks !

@dhantule dhantule added type:Bug stat:awaiting response from contributor and removed type:support User is asking for help / asking an implementation question. Stackoverflow would be better suited. labels Dec 11, 2024
@yubori
Copy link
Author

yubori commented Dec 13, 2024

Hi @dhantule
I am looking forward to it being fixed. Thank you.

@dhantule dhantule added the keras-team-review-pending Pending review by a Keras team member. label Dec 18, 2024
@mattdangerw
Copy link
Member

I think in general we should probably let numpy be the guide here. np.eye will not accept floats or numpy array values, the same is true for jax and torch. This seems correct to me.

The bug is that tf is too permissive here, we could add errors so that keras.ops.eye errors for floats on the tf backend similar to jax and torch.

I'll mark this as contributors welcome. This is a good first issue to bring tf in line with jax and torch.

@mattdangerw mattdangerw added stat:contributions welcome A pull request to fix this issue would be welcome. Good first issue Issues which are good for first-time contributors. Usually easy to fix. backend:tensorflow and removed keras-team-review-pending Pending review by a Keras team member. labels Dec 19, 2024
rujutajoshi232 added a commit to rujutajoshi232/keras that referenced this issue Dec 23, 2024
@rujutajoshi232 rujutajoshi232 linked a pull request Dec 23, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:tensorflow Good first issue Issues which are good for first-time contributors. Usually easy to fix. stat:contributions welcome A pull request to fix this issue would be welcome. type:Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants