Skip to content
This repository was archived by the owner on Nov 24, 2018. It is now read-only.

Conversation

btracey
Copy link
Member

@btracey btracey commented Feb 25, 2016

No description provided.

@btracey btracey mentioned this pull request Feb 25, 2016
native/dsteqr.go Outdated
if anorm == 0 {
goto Ten
}
// TODO(btracey): Replace this with a constant
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is "this"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it was a note to myself to have the same up/down constants. I replaced this.

@btracey
Copy link
Member Author

btracey commented Feb 26, 2016

PTAL

native/dsteqr.go Outdated
if anorm == 0 {
goto Ten
}
if anorm > ssfmax {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be a switch too, and clean up the line ordering to be consistent between the two cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@kortschak
Copy link
Member

LGTM but wait for @vladimir-ch.

return true
}
if n == 1 {
if icompz == 2 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just being curious: why isn't z[0] set to 1 if icompz == 1? The documentation says that it will also contain orthonormal eigenvectors. Sorry if the reason is obvious.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure the code is correct. The reason is that if compz == 'V' then z already contains a valid diagonalization (according to any of the metrics specified). If compz == 'I', then z can contain anything and z must be set to I. Since size(z) = 1, it's just the first element.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the doc comment says that z contains something only if compz == lapack.EigBoth (compz == 'I').

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation is incorrect. Fixed.

@vladimir-ch
Copy link
Member

LGTM

btracey added a commit that referenced this pull request Mar 2, 2016
@btracey btracey merged commit 896040d into master Mar 2, 2016
@btracey btracey deleted the adddsteqr branch March 2, 2016 02:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants