-
Notifications
You must be signed in to change notification settings - Fork 15
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
LowRankFactorizationNEP to NEPTypes #158
Conversation
This should handle #157 |
I'm going to merge this now. A note for future: we can consider using the |
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.
Sorry for the delayed response, I think these changes look good, I had just a couple of stylistic suggestions which shouldn't affect functionality or performance.
nep1=SPMF_NEP([Dn-Vn,In],[f1,f2]); | ||
|
||
# Second NEP. Construct as a LowRankFactorizedNEP | ||
nep2=SPMF_NEP([G,F],[g,f]) |
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.
I guess this could be removed now? Along with G
, and the definition of F
could optionally be done directly into Uv2
.
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.
Thanks. I've fixed it in master.
|
||
|
||
end | ||
|
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.
We could consider simplifying these methods a bit, and even combining them to get rid of the duplicated code. I like to use higher order functions and fusing dots since it usually handles type stability and avoids unnecessary allocations. I think the above two methods could be written as a single method like this:
function LowRankFactorizedNEP(L::AbstractVector{S}, U::AbstractVector{S}, f, A = L .* adjoint.(U)) where {S<:AbstractMatrix}
rank = mapreduce(u -> size(u, 2), +, U)
return LowRankFactorizedNEP{S}(SPMF_NEP(A, f, align_sparsity_patterns=true), rank, L, U)
end
Alternatively, keeping the two methods if we don't want to compute A
as part of the argument list.
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.
Thx. I went for two functions.
@@ -52,6 +53,29 @@ function LowRankMatrixAndFunction(A::AbstractMatrix{<:Number}, f::Function) | |||
LowRankMatrixAndFunction(A, L, U, f) | |||
end | |||
|
|||
export LowRankFactorizedNEP | |||
|
|||
# Additional constructor for particle example |
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.
Alternatively, with higher order functions and dot notation:
function LowRankFactorizedNEP(Amf::AbstractVector{LowRankMatrixAndFunction{S}}) where {T<:Number, S<:AbstractMatrix{T}}
# if A is not specified, create it from LU factors
A = [isempty(M.A) ? M.L * M.U' : M.A for M in Amf]
L = getfield.(Amf, :L)
U = getfield.(Amf, :U)
f = getfield.(Amf, :f)
rank = mapreduce(M -> size(M.U, 2), +, Amf)
return LowRankFactorizedNEP(SPMF_NEP(A, f, align_sparsity_patterns=true), rank, L, U)
end
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.
Sweet. Fixed in master.
|
||
for k = 1:q | ||
# if A is not specified, create it from LU factors | ||
A[k] = L[k] * U[k]' |
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.
Btw, does it make sense to use the conjugate transpose here? I believe that I did it this way since that's how it was implemented in the original NLEIGS code, but would this be preferred (and not confusing) over just A = L * U
, without transpose?
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.
I think it's just because linear algebra researcher tend to prefer tall skinny matrices over short and fat. It's the same in LowRankMatrix in LowRankApprox-package. https://github.com/JuliaMatrices/LowRankApprox.jl/blob/master/src/lowrankmatrix.jl . To me what is currently called low rank matrix (both in our code and LowRankApprox) has not so much to do with the rank, but just a representation of a product of two matrices. A better name is maybe just be MatrixProductNEP, where every term is product of two matrices. Such a change would be too big for right now.
Moved the
LowRankFactorizedNEP
to NEPTypes. Madeschrodinger_movebc
use it + added a bit of functionality. Not in this PR: Some of the compute functions can be made more efficient by not constructing the full matrix, but only working with the factorizations. Some more Gallery examples could benefit from using this (as commented in the gallery).I'm adding max as reviewer as courtesy. I know you're busy.