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
Structured opnorm #30138
base: master
Are you sure you want to change the base?
Structured opnorm #30138
Conversation
stdlib/LinearAlgebra/src/bidiag.jl
Outdated
return convert(Tnorm, nrm) | ||
end | ||
|
||
function _loopnorm1(A::Bidiagonal{T}) where T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lower bidiagonal p=1 norm is the same code as upper bidiagonal p=Inf (and the same with upper p=1 and lower p=Inf). These can be combined but that would reduce readability in my opinion. Thoughts on how to make this cleaner?
One option is to do _upnormInf(A::Bidiagonal) = _loopnorm1(A)
and _lonormInf(A::Bidiagonal) = _upnorm1(A)
and document it well.
Thanks for adding these optimizations. However, I think it would be good to write them with a function _opnorm1Inf(B::Bidiagonal, p)
case = xor(p == 1, istriu(B))
normd1, normdend = norm(first(B.dv)), norm(last(B.dv))
normd1, normdend = case ? (zero(normd1), normdend) : (normd1, zero(normdend))
return max(mapreduce(t -> sum(norm, t), max, zip(view(B.dv, (1:length(B.ev)) .+ !case), B.ev)), normdend)
end What do you think? Currently, there is a slight overhead from using this formulation but it's not much and nothing if you compare to the current situation. It is also likely that the different will vanish over time as more optimizations will be added to the language. |
That is quite nice, thanks for the suggestion. There are some edge cases (it fails for a 1x1 matrix) but these can be worked around. One question that I have is how it will be better for |
Yes. Using explicit loops and indexing is often only useful for
I agree to some extend. A loop is very easy to read but all the (necessary) promotion nonsense (that I've written a lot of myself) is a distraction and is nice to hide in other functions when possible. It is also a lot of repetition of almost the same code. As you mention, it is possible to reuse some of the methods by flipping the upper/lower and 1 to Inf. However, the main argument for using |
Bump. It would be great to get this merged in some form. Do you think it can be rewritten to use higher-order functions? Otherwise please try to reuse as much as possible as you suggested. |
@andreasnoack Hi, yes I will try to rewrite it. Are you aiming for this to be included in the upcoming backports? I am a bit busy until the end of this month.. |
The plan is to have another minor release in about four months so you can just finish this when time allows. It would just be a shame if it stalls completely. |
Having it in January would be greaat—then it will be easily included in 1.2 with plenty of time for testing. |
@andreasnoack Do you mind taking a look at this? |
Bump |
@andreasnoack: do you have bandwidth to take a look at this? @mcognetta, can you rebase this and fix the conflict so that we can merge if it passes CI? |
Got it. There is a stylistic change that may be nice to consider. There are currently 4 nearly identical methods like: function opnorm(B::Bidiagonal, p::Real=2)
if p == 2
return opnorm2(B)
elseif p == 1 || p == Inf
return _opnorm1Inf(B, p)
else
throw(ArgumentError("invalid p-norm p=$p. Valid: 1, 2, Inf"))
end
end This could be reduced to 2 (or even 1, if we change the arguments for the symmetric structured matrices) by making something similar to: function opnorm(A::Union{Tridiagonal, Bidiagonal}, p::Real=2)
if p == 2
return opnorm2(A)
elseif p == 1 || p == Inf
return _opnorm1Inf(A, p)
else
throw(ArgumentError("invalid p-norm p=$p. Valid: 1, 2, Inf"))
end
end Not sure where I would place those methods though. |
It would be great to combine the methods when possible. You can put them in |
I combined the callers for |
making a test case use `isapprox` instead of `==`.
sorry for another bump |
Not at all. Thank you for sticking with it! Can we get a final review on this and merge it? |
Bumping this. It seems #32093 added |
Needs conflicts resolved as well. |
I resolved the merge conflicts. I will start a PR for the |
Anything I can do to help here? this bit me today... A = SymTridiagonal(randn(1000000),randn(999999)); @time opnorm(A,1) is a fun command to run... |
Oh I thought that this was done. I'll finish it up and get it submitted. @dgleich any other optimizations you can see here? Open to suggestions. |
Just some high level comments ...
|
opnorm
falls back to generic methods for structured matrices (Diagonal
,Bidiagonal
,Tridiagonal
,SymTridiagonal
). For thep=1
andp=Inf
cases, this is usually just a regular sum over the rows and columns which can be sped up. Thep=2
case falls back tosvdvals!
for which there are specialized methods for each of the types except forTridiagonal
andSymTridiagonal
(see #30137).This implements faster
p=1
andp=Inf
opnorms for structured matrices but leaves thep=2
case broken forTridiagonal
andSymTridiagonal
.Some times:
v1.0
This PR