Fixed division by zero in QR decomposition. Issue #1058

[jarno-r]

Description

tensorflow/tfjs#1058
The sign() function returns 0 on 0, which causes a division by zero in the QR decomposition function qr() if there is a zero on the diagonal.

---

For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md

---

This change is 

[jarno-r]

FYI: #1366 has a new implementation of qr() using an entirely different algorithm, which might also fix this issue.

[caisq]

Reviewable status: 0 of 1 approvals obtained (waiting on @jarno-r)

---

src/ops/linalg_ops.ts, line 217 at r1 (raw file):

greater(tensor2d([[0]]))

You can just do rjj.greater(0) here.

[DirkToewe]

I'm not sure I understand, why there is a sign there in first place. Isn't it completely arbitrary wether or not we Householder reflect to [..., -norm, 0, ..., 0] or [..., +norm, 0, ..., 0]? It makes the gradient smother when the input is close to triangular but:

• The gradient cannot be computed as is anyways, since dispose() is called manually
• The gradient becomes quite volatile if the diagonal entry is close to zero, since the sign of the diagonal might change on tiny pertubations while the norm might still be huge. The gradient in those cases is close to infinity.

[jarno-r]

@caisq Change committed.
@DirkToewe The code appears to be based directly on the lecture notes mentioned in the comments. There's no explanation there, why the sign is chosen as is. There's a somewhat cryptic note on the Wikipedia page on QR decomposition via Householder reflections claiming that the sign should be chosen to avoid losing significance, but that doesn't make any sense to me.
I really don't know much about how tensorflow.js works, so I can't say anything about the use of dispose().

[DirkToewe]

Oh, that actually makes sense. If You subtract a large float form another equally large float the result is a small float that only represents the last few digits of the two large numbers so there is a loss in significance. So it makes sense from an accuracy standpoint. Since gradients can't be computed anyways yet, there is no valid argument against it either.

[jarno-r]

Reviewable status: 0 of 1 approvals obtained (waiting on @caisq)

---

src/ops/linalg_ops.ts, line 217 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…

greater(tensor2d([[0]]))

You can just do rjj.greater(0) here.

Done.

[caisq]

Thank you, @DirkToewe !

Reviewable status: 0 of 1 approvals obtained (waiting on @caisq)

[DirkToewe]

@caisq I believe, You meant to thank @jarno-r

[caisq]

That's correct, @DirkToewe :)

[DirkToewe]

@jarno-r I believe it's up to You to commit this PR. @caisq had given You the GTG.

[jarno-r]

@DirkToewe It says 'Only those with write access to this repository can merge pull requests.' I don't think I have write access.

[dsmilkov]

Thank you! Apologies for the long wait here!

Reviewed 2 of 2 files at r4.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @caisq)

[googlebot]

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 @googlebot I consent. in this pull request.

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 cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[googlebot]

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

[dsmilkov]

CLA is ok since the commits are mine due to sync with master.

[jarno-r]

@googlebot I consent.