Skip to content

Conversation

@andreasnoack
Copy link
Member

Sinv = zeros(Stype, length(SVD.S))
index = SVD.S .> rtol*maximum(SVD.S)
Sinv[index] = one(Stype) ./ SVD.S[index]
Sinv[findall(.!isfinite.(Sinv))] .= zero(Stype)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not needed for this change. However, zero(Stype) isn't needed since the array is already allocated with Stype elements and 0 is easier to read than zero(Stype).

Copy link
Member

Choose a reason for hiding this comment

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

Aren't there places where zero(T) works but convert(T, 0) fails? I'd let sleeping dogs lie here, especially since we also use one and zeros immediately surrounding this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are but they don't apply here since Stype is the type the singular values. It's mainly for non-number types like arrays that you can't rely on converting from 0. This change is unrelated to the PR so I've removed it.

Copy link
Member

Choose a reason for hiding this comment

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

I had also been thinking of unitful numbers but hadn't worked out if/how unitful SVDs work. In any case, thanks for indulging my conservativeness.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is true. The singular values have units so that would fail. Unfortunately, it would be a bit complicated to make the svd solver in GenericLinearAlgebra work for Unitful.Quantity because it's only a subtype of Number.

@andreasnoack andreasnoack merged commit a472bc7 into master Oct 30, 2018
@andreasnoack andreasnoack deleted the an/pinv branch October 30, 2018 10:12
KristofferC pushed a commit that referenced this pull request Oct 31, 2018
Fixes #29723

(cherry picked from commit a472bc7)
KristofferC pushed a commit that referenced this pull request Oct 31, 2018
Fixes #29723

(cherry picked from commit a472bc7)
KristofferC pushed a commit that referenced this pull request Nov 2, 2018
Fixes #29723

(cherry picked from commit a472bc7)
KristofferC pushed a commit that referenced this pull request Feb 11, 2019
Fixes #29723

(cherry picked from commit a472bc7)
KristofferC pushed a commit that referenced this pull request Feb 20, 2020
Fixes #29723

(cherry picked from commit a472bc7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

linear algebra Linear algebra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pinv of lazy adjoints and transposes?

3 participants