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

Implement eagerpy.diag #22

Closed
wants to merge 3 commits into from
Closed

Conversation

Holt59
Copy link

@Holt59 Holt59 commented Oct 23, 2020

Closes #21

Implements eagerpy.diag, following numpy / torch convention:

  • Given a one-dimensional tensor, returns a two-dimensional tensor whose diagonal contains the input tensor values, and all other entries are 0. The second (optional) parameter indicates which diagonal to set.
  • Given a two-dimensional tensor, returns a one-dimensional tensor containing the values in the diagonal of the input tensor. The second (optional) parameter indicates which diagonal to get.

This cannot handle batched inputs like Tensorflow linalg.diag or linalg.diag_part, but I think it's cleaner that way, let me know what you think.

@Holt59
Copy link
Author

Holt59 commented Oct 23, 2020

@jonasrauber
Copy link
Owner

jonasrauber commented Oct 24, 2020

Let's call the method diag not _diag. No need limit diag to ep.diag. Might be nice to chain diag like other methods.

@Holt59
Copy link
Author

Holt59 commented Oct 24, 2020

Let's call the method diag not _diag. No need limit diag to ep.diag. Might be nice to chain diag like other methods.

Done. I've also reverted the change to test_transpose_1d I made by mistake.

@jonasrauber
Copy link
Owner

Thanks! Seems a couple of tests fail because the frameworks return inconsistent results.

@Holt59
Copy link
Author

Holt59 commented Oct 26, 2020

Thanks! Seems a couple of tests fail because the frameworks return inconsistent results.

Yes, as I said in a previous comment, the tensorflow version used in the CI tests has a bug regarding the k parameter in tf.linalg.diag (see the link in the previous comment).

@Holt59
Copy link
Author

Holt59 commented Jul 2, 2021

I've rebased the PR from master to get the upgraded TF in tests, hopefully this should fix the bug previously mentioned.

@Holt59 Holt59 closed this Sep 29, 2023
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.

Equivalent of np.diag?
2 participants