Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add COCG method for complex symmetric linear systems #289
base: master
Are you sure you want to change the base?
Add COCG method for complex symmetric linear systems #289
Changes from 10 commits
c5b440e
91208cd
3337884
f7df543
d5b31f4
97315be
593e88c
835a894
9c47e7f
3cd7969
d16fecb
04c3c16
4800129
9a7fb26
f3710c3
b457247
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
You should say that this is for the case of complex-symmetric (not Hermitian) matrices
A
.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.
this shouldn't allocate, at least not on julia 1.5 and above; so let's drop that comment.
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 is allocating when
x
andy
are not vectors; see my previous comment #289 (comment). The test in the comment was done with Julia 1.5.4. Could you confirm if it is non-allocating forx
andy
that are multi-dimensional arrays?If it is allocating, how about changing the comment in the code from
# allocating, but ...
to# allocating for non-vectorial x and y, but ...
?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, but I'm not following why you're using
view(x, :)
in the first place.x
andy
are assumed to behave as vectors; we have no block methods.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.
and the only thing that might allocate if julia is not smart enough is the array wrapper, but it's O(1) not O(n), so not worth documenting
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.
What if
x
andy
are multi-dimensional arrays? It is quite common for iterative methods to handle suchx
andy
with matrix-free operatorsA
acting on them.It is true that multi-dimensional arrays
x
andy
are vector-like in a sense that vector operators such asdot(x, y)
are defined on them. Other methods inIterativeMethods
must have worked fine so far because they relied only on such vector operators. Unfortunately the unconjugated dot product is not one of such vector operators in Julia, so I had to turn those vector-like objects into actual vectors using the wrapper@view
.If the unconjugated dot product had been one of those operators required for making objects behave as vectors, I wouldn't have had to go through these complications...
Fair enough. I will remove the comment in the next commit.
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.
The CG method only solves
Ax=b
right now where x and b are vectors, notAX=B
for multiple right-hand sides. I think we have to reserve B::AbstractMatrix for the use case of block methods if they ever get implemented.Orthogonal to that is how you internally represent your data. If you want X and B to be a tensor because that maps well to your PDE finite differences scheme, then the user should still wrap it in a vector type
and define the interface for it.
Or you don't and you call
cg(my_operator, reshape(B, :))
wheremy_operator
restores the shape to B's.But generally we assume the interface of vectors in iterative methods, and later we can think about supporting block methods to solve AX=B for multiple right-hand sides, where we assume a matrix interface.
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.
Not sure if we should support multiple right-hand sides with the same
cg()
interface. The currentcg()
implementation interprets a non-vectorb
(includingb::AbstractMatrix
) as a single right-hand-side column vector, as long as*
is defined between the givenA
andb
. I think supporting such usage in the high-level interface inIterativeSolvers.jl
was the conclusion people reached after the lengthy discussion in #2 about duck-typing.That said, my current proposal
is not the perfect solution that supports the most general type of
x
either:_norm
assumes iterablex
and_dot
assumes reshapablex
, and the Banach space element mentioned in #289 (comment) satisfies neither condition. The proposal just makescocg()
works for the most popular cases ofx::AbstractArray
without requiring the users to define_norm(x, ::UnconjugatedDot)
and_dot(x, y, ::UnconjugatedDot)
separately. For other types ofx
, the users must define_norm
and_dot
for the type.(I think the best way to solve the present issue might be to define a lazy wrapper
Conjugate
likeTranspose
orAdjoint
in the standard library and rewriteLinearAlgebra:dot
to interpretdot(Conjugate(x), y)
as the unconjugated dot product.)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.
@haampie and @stevengj, any thoughts on this?