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

Missing op: MatrixBandPart #1969

Closed
annxingyuan opened this issue Sep 4, 2019 · 8 comments
Closed

Missing op: MatrixBandPart #1969

annxingyuan opened this issue Sep 4, 2019 · 8 comments
Labels

Comments

@annxingyuan
Copy link
Collaborator

Reference from TensorFlow: https://www.tensorflow.org/api_docs/cc/class/tensorflow/ops/matrix-band-part

This op is used by the Music Transformer model.

@Kriyszig
Copy link
Contributor

Kriyszig commented Oct 4, 2019

Is this issue taken?
If not I can get an implementation ready by tomorrow hopefully!

@annxingyuan annxingyuan added the good first issue Good for newcomers label Oct 4, 2019
@annxingyuan
Copy link
Collaborator Author

@Kriyszig Fantastic! It's all yours :)

@DirkToewe
Copy link
Contributor

DirkToewe commented Oct 15, 2019

PR#1520 from January already implemented tf.linalg.bandPart. It is purely based on existing tfjs operations and, according to some quick and dirty benchmarks, it is around 10 times slower than a backend-based implementation, which should be acceptable for an O(m*n) operation. I've taken the liberty to add PR#2155 to the benchmark here. PR#2155 currently seems to have some performance issues caused by the way that concat is used.

@Kriyszig
Copy link
Contributor

@DirkToewe, Can you port PR#1520 to this monorepo? Your implementation scales really well unlike mine.

@DirkToewe
Copy link
Contributor

Okay, will do as soon as I can.

@DirkToewe
Copy link
Contributor

DirkToewe commented Oct 17, 2019

Done! You can find the PR here. @Kriyszig Your PR had better documentation and more tests so I've based my PR on it. If that's okay with You, You just need to to give Your consent in the PR.

@hrldcpr
Copy link

hrldcpr commented Feb 6, 2020

Seems like this can be closed now, thanks to #2226?

@rthadur
Copy link
Contributor

rthadur commented Aug 11, 2021

Closing this issue as the related PR has been merged.

@rthadur rthadur closed this as completed Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants