tf.linalg.bandPart #2226
tf.linalg.bandPart #2226
Conversation
* Added bandPart operation from Tensorflow to Tfjs
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
Great work @DirkToewe |
|
@googlebot I consent |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
Thank you, @Kriyszig @DirkToewe ! I will try to review this within the next few days! |
tfjs-core/src/ops/linalg_ops.ts
Outdated
|
|
||
| return ENGINE.tidy( () => { | ||
| const $a = convertToTensor(a,'a','bandPart'); | ||
| a = undefined; |
There was a problem hiding this comment.
Why set a to undefined?
There was a problem hiding this comment.
The idea was to prevent myself from using a instead of $a in the remaining lines of code. It was meant for assertion purposes only and does not belong in the final commit. I'm going to remove it.
There was a problem hiding this comment.
If I am not mistaken, the ENGINE.tidy is redundant, right? op({_bandPart}) already adds a scope which disposes the local tensors. I've removed it while I was at it (hope I'm not mistaken).
There was a problem hiding this comment.
Yes, the ENGINE.tidy would have been redundant. Thanks!
* Removed ENGINE.tidy from _bandPart() (op({_bandPart})) already tidies up
* Removed three cases of duplicate tests
|
I've changed Thank You so much for taking the time for the code reviews. |
|
hey @DirkToewe I'm having trouble viewing our CI logs right now... could you just make sure |
|
Just checked one more time:
The CI tests seem to have passed now. Maybe there is an issue with tests timing out? Edit: I could try to reduce the number of assertions in the last test and/or split it up. |
Implementation of
tf.linalg.bandParta.k.a.tf.matrixBandPart, based purely on other TFJS ops. No additional backend kernels are added. The performance should still be goodish enough compared to custom kernels.@Kriyszig deserves credit for the documentation and most of the tests.
This change is