Skip to content
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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

wsshin
Copy link

@wsshin wsshin commented Jan 25, 2021

This PR adds the conjugate orthogonal conjugate gradient (COCG) method for complex symmetric matrices A (i.e., transpose(A) == A rather than adjoint(A) == A). The PR implements cocg and cocg! functions (corresponding to cg and cg! for Hermitian matrices), supporting functions and types, and basic unit tests.

This PR is related with JuliaLang/julia#288.

@wsshin
Copy link
Author

wsshin commented Jan 25, 2021

I haven't added any documentation pages for COCG, as IterativeSolvers.jl seems to use Documenter.jl, which I haven't used before. Please let me know if there are some basic guidelines as to how to create documentation pages using Documenter.jl.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 508517998

  • 0 of 63 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.3%) to 97.198%

Totals Coverage Status
Change from base Build 444205317: -0.3%
Covered Lines: 1873
Relevant Lines: 1927

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jan 25, 2021

Pull Request Test Coverage Report for Build 652039845

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 17 of 17 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 97.83%

Totals Coverage Status
Change from base Build 444205317: 0.3%
Covered Lines: 1939
Relevant Lines: 1982

💛 - Coveralls

@haampie
Copy link
Member

haampie commented Jan 25, 2021

Hi @wsshin, thanks for your contribution!

I think another way to do this is to dispatch to a different dot-function inside of CG based on a tag.

For instance:

struct InnerProduct end
struct UnconjugatedProduct end

_norm(::InnerProduct, xs) = norm(xs)
_dot(::InnerProduct, xs, ys) = dot(xs, ys)

_norm(::UnconjugatedProduct, xs) = sqrt(sum(x^2 for x in xs))
_dot(::UnconjugatedProduct, xs, ys) = sum(prod, zip(xs, ys))

and then add this tag InnerProduct / NoInnerProduct to the CG iterable type. That saves some duplication.

Is there a name for this non-conjugate innerproduct-like operation sum(prod, zip(xs, ys))?

Copy link
Member

@haampie haampie left a comment

Choose a reason for hiding this comment

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

See above, would be nice to reduce duplication a bit.

@wsshin
Copy link
Author

wsshin commented Jan 25, 2021

@haampie, thanks for the suggestion. Please let me know if the new commit fulfills your request.

Copy link
Author

@wsshin wsshin left a comment

Choose a reason for hiding this comment

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

@haampie, I submitted new commits implementing your ideas. Please take a look and let me know what you think.

@wsshin
Copy link
Author

wsshin commented Jan 26, 2021

Not sure why codecov generated a failure. Tried to remove it by adding more unit tests, but it didn't work. Please advise.

@wsshin wsshin requested a review from haampie January 27, 2021 14:15
@wsshin
Copy link
Author

wsshin commented Jan 28, 2021

@haampie, any thoughts on the new commit in this PR? Hope to finish this PR soon and move on to the implementation of COCR as mentioned in JuliaLang/julia#288.

@haampie
Copy link
Member

haampie commented Jan 28, 2021

Hi @wsshin , i'll review in the weekend if that's ok with you.

src/cg.jl Outdated
@@ -47,18 +57,19 @@ function iterate(it::CGIterable, iteration::Int=start(it))
end

# u := r + βu (almost an axpy)
β = it.residual^2 / it.prev_residual^2
ρ = _dot(it.r, it.conjugate_dot)
Copy link
Member

@haampie haampie Jan 31, 2021

Choose a reason for hiding this comment

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

CG and preconditioned CG are split into separate methods because CG requires one fewer dot product. Can you check if you can drop this dot in your version somehow?

Copy link
Author

Choose a reason for hiding this comment

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

Good point, but I'm afraid we can't. It turns out that the saving of one dot product in the non-preconditioned CG comes from the fact that we calculate ρ = r⋅r and the norm of the residual vector can be calculated by sqrt(ρ) instead of performing the dot product again.

In COCG, we use the unconjugated dot product, so ρ = transpose(r)*r. Therefore, the norm of the residual vector cannot be calculated by sqrt(ρ), but needs to be calculated by sqrt(r⋅r).

Copy link
Member

Choose a reason for hiding this comment

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

In that case you should probably have

β = it.conjugate_dot isa Val{true} ? it.residual^2 / it.prev_residual^2 : _dot(it.r, it.conjugate_dot)

so that it only does the extra dot product in the unconjugated case.

Copy link
Member

Choose a reason for hiding this comment

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

And if you do that please change Val{true} into a named type to improve the readability a bit

Copy link
Author

Choose a reason for hiding this comment

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

Done!

src/cg.jl Outdated
end

mutable struct PCGIterable{precT, matT, solT, vecT, numT <: Real, paramT <: Number}
mutable struct PCGIterable{precT, matT, solT, vecT, numT <: Real, paramT <: Number, boolT <: Union{Val{true},Val{false}}}
Copy link
Member

Choose a reason for hiding this comment

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

Can you turn it into two named structs instead of Val{true/false}, it would be easier to read.

Copy link
Author

@wsshin wsshin Jan 31, 2021

Choose a reason for hiding this comment

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

I understand defining InnerProduct and UnconjugatedProduct was your original suggestion, but I thought passing a boolean flag in the top-level user interface like

cg!(x, A, b; conjugate_dot=true)

would be easier for the users than something like

cg!(x, A, b; innerprodtype=UnconjugatedProduct())

Introduction of a new type should be minimized, because it reduces portability. Also, I don't think using a boolean flag reduces readability in this case, because the flag is a keyword argument.

If we agree that passing a boolean flag conjugate_dot to the top interface is a slightly better choice, it would be more natural to do

CGIterable(A, x, r, ..., Val(conjugate_dot))

than

CGIterable(A, x, r, ..., conjugate_dot ? InnerProduct() : UnconjugatedProduct())

This is why I decided to include a boolean type in CGIterable than a completely new type as you suggested originally. Please let me know what you think

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking the high-level interface could just be cocg! instead of cg! with a flag. Would be nice if the methods would be type stable without relying on constant propagation of conjugate_dot.

Copy link
Member

Choose a reason for hiding this comment

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

Introduction of a new type should be minimized, because it reduces portability.

I'm not sure what you mean by "reducing portability," Wonseok?

A new type has the advantage of allowing more than two possibilities. For example, one could imagine a WeightedDot(B) that corresponds to the dot(x,B,y) dot product — this is useful in cases where A is Hermitian with respect to some nonstandard dot product (the alternative being to perform a change of variables via Cholesky factors of B, which may not always be convenient).

conjugate_dot=true has the disadvantage of potentially requiring a dynamic dispatch at the top level. (Unless the compiler does constant propagation, but I think it only does that if the function is short enough to be inlined, which I don't think cg! is?) That's not such a big deal here, however, because overhead of a single dynamic dispatch should be negligible for large problems where iterative solvers are used.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure what you mean by "reducing portability," Wonseok?

Please ignore my comment on portability. I think it was irrelevant to the current situation. (For clarification and my future reference, I meant it is not desirable to define a package-specific collection type that aggregates various fields and to pass such a collection to a function in order to reduce the number of function arguments.)

A new type has the advantage of allowing more than two possibilities. For example, one could imagine a WeightedDot(B) that corresponds to the dot(x,B,y) dot product — this is useful in cases where A is Hermitian with respect to some nonstandard dot product (the alternative being to perform a change of variables via Cholesky factors of B, which may not always be convenient).

I really like this flexibility!

conjugate_dot=true has the disadvantage of potentially requiring a dynamic dispatch at the top level. (Unless the compiler does constant propagation, but I think it only does that if the function is short enough to be inlined, which I don't think cg! is?) That's not such a big deal here, however, because overhead of a single dynamic dispatch should be negligible for large problems where iterative solvers are used.

I didn't know that constant propagation can be a means to avoid dynamic dispatch. Glad to learn a new thing. Thanks for the information! (I find this useful on this topic.)

But before knowing about constant propagation, I thought the the overhead of a dynamic dispatch should be negligible when iterative solvers are useful, as you pointed out.

I have pushed a commit complying with this request.

src/cg.jl Outdated
@@ -2,20 +2,29 @@ import Base: iterate
using Printf
export cg, cg!, CGIterable, PCGIterable, cg_iterator!, CGStateVariables

mutable struct CGIterable{matT, solT, vecT, numT <: Real}
# Conjugated dot product
_dot(x, ::Val{true}) = sum(abs2, x) # for x::Complex, returns Real
Copy link
Member

Choose a reason for hiding this comment

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

Maybe better to just use _norm / norm; that's what was used before and norm is slightly more stable. Also I'm not sure if sum(abs2, x) works as efficiently on GPUs, norm is definitely optimized.

Copy link
Author

@wsshin wsshin Jan 31, 2021

Choose a reason for hiding this comment

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

What do you mean by "norm is more stable"? If you are talking about the possibility of overflow, I think it doesn't matter because the calculated norm is squared again in the algorithm.

Also, for the unconjugated dot product, the norm(x) function would return sqrt(xᵀx), which is not a norm because xᵀx is complex. In COCG the quantity we use is xᵀx, not sqrt(xᵀx), so I don't feel that it is a good idea to store the square-rooted quantity and then to square it again when using it.

I verify that the GPU performance of sum does not degrade.

julia> using LinearAlgebra, CuArrays, BenchmarkTools

julia> v = rand(ComplexF64, 10^9);

julia> cv = cu(v);

julia> @btime norm($v);
  1.890 s (0 allocations: 0 bytes)

julia> @btime sum(abs2, $v);
  1.452 s (0 allocations: 0 bytes)  # sum() is significantly faster than norm() on CPU

julia> @btime norm($cv);
  18.341 ms (1 allocation: 16 bytes)

julia> @btime sum(abs2, $cv);
  18.341 ms (361 allocations: 13.94 KiB)  # sum is on par with norm() on GPU

Copy link
Member

@stevengj stevengj Mar 5, 2021

Choose a reason for hiding this comment

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

The main advantage of falling back to norm and dot in the conjugated case is that they generalize better — if the user has defined some specialized Banach-space type (and corresponding self-adjoint linear operators A with overloaded *), they should have overloaded norm and dot, whereas sum(abs2, x) might no longer work.

(Even as simple a generalization as an array of arrays will fail with sum(abs2, x), whereas they work with dot and norm.)

The overhead of the additional sqrt should be negligible.

Copy link
Member

Choose a reason for hiding this comment

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

1.452 s (0 allocations: 0 bytes) # sum() is significantly faster than norm() on CPU

This is because it dispatches to BLAS which does a stable norm computation. But generally BLAS libs are free to choose how to implement norm.

Copy link
Author

@wsshin wsshin Mar 6, 2021

Choose a reason for hiding this comment

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

The main advantage of falling back to norm and dot in the conjugated case is that they generalize better — if the user has defined some specialized Banach-space type (and corresponding self-adjoint linear operators A with overloaded *), they should have overloaded norm and dot, whereas sum(abs2, x) might no longer work.

(Even as simple a generalization as an array of arrays will fail with sum(abs2, x), whereas they work with dot and norm.)

This is a very convincing argument. Additionally, considering that a user-defined Banach-space type should overload norm and dot but not the unconjugated dot, it will be nice to be able to define the unconjugated dot using dot like

_dot(x, y, ::UnconjugatedDot) = dot(conj(x), y)

but of course this is inefficient as it conjugates x twice, not to mention extra allocations. I wish dotu was exported, as you suggested in https://github.com/JuliaLang/julia/issues/22227#issuecomment-306224429.

Users can overload _dot(x, y, ::UnconjugatedDot) for the Banach-space type they define, but what would be the best way to minimize such a requirement? I have pushed a commit implementing the unconjugated dot with sum, but if there is a better approach, please let me know.

Copy link
Member

Choose a reason for hiding this comment

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

You could implement the unconjugated dot as transpose(x) * y ... this should have no extra allocations since transpose is a "view" type by default?

Copy link
Member

Choose a reason for hiding this comment

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

Though transpose(x) * y wouldn't work if x and y are matrices (and A is some kind of multlinear operator); maybe it's best to stick with the sum(zip(x,y)) solution you have now.

Copy link
Author

Choose a reason for hiding this comment

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

Can we assume that getindex() is always defined for x and y? Then how about using transpose(@view(x[:])) * @view(y[:])? This uses allocations, but it seems much faster than sum(prod, zip(x,y)):

julia> x = rand(100,100); y = rand(100,100);

julia> @btime sum(prod, zip($x,$y));
  12.564 μs (0 allocations: 0 bytes)

julia> @btime transpose(@view($x[:])) * @view($y[:]);
  1.966 μs (4 allocations: 160 bytes)

It is nearly on a par with dot:

julia> @btime dot($x,$y);
  1.839 μs (0 allocations: 0 bytes)

Also, when x and y are AbstractVectors, the proposed method does not use allocations:

julia> x = rand(1000); y = rand(1000);

julia> @btime transpose(@view($x[:])) * @view($y[:]);
  108.091 ns (0 allocations: 0 bytes)

Copy link
Member

Choose a reason for hiding this comment

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

You could do dot(transpose(x'), x), maybe?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't know that x' was nonallocating! This looked like a great nonallocating implementation, but it turns out that this is slower than the earlier proposed solution using @view:

julia> @btime transpose(@view($x[:])) * @view($y[:]);
  267.794 ns (0 allocations: 0 bytes)

julia> @btime dot(transpose($x'), $y);
  1.547 μs (0 allocations: 0 bytes)

Also, neither x' nor transpose(x) works for x whose dimension is greater than 2.

I just pushed an implementation using @view for now.

@wsshin
Copy link
Author

wsshin commented Feb 7, 2021

@haampie, please let me know if you don't agree with my opinions; I can accommodate yours.

@wsshin wsshin requested a review from haampie February 13, 2021 16:10
The usage of the unconjugated dot product is not limited to implementing
COCG from CG, so it is more appropriate to move its definition outside
cg.jl.  For example, the unconjugated dot product can be used in
implementing COCR from CR.
@wsshin wsshin mentioned this pull request Feb 22, 2021
@wsshin
Copy link
Author

wsshin commented Mar 6, 2021

I think I have addressed all the concerns raised so far. If there is anything else, please let me know.

src/common.jl Outdated
"""
Conjugated and unconjugated dot products
"""
abstract type DotType end
Copy link
Member

Choose a reason for hiding this comment

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

I would call this AbstractDot in keeping with the usual Julia naming conventions.

src/cg.jl Outdated
maxiter::Int
mv_products::Int
dot_type::dotT
Copy link
Member

Choose a reason for hiding this comment

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

Maybe dotproductdot_type is a bit inapt since it is not a Type.

src/common.jl Outdated
_dot(x, y, ::ConjugatedDot) = dot(x, y)

_norm(x, ::UnconjugatedDot) = sqrt(sum(xₖ^2 for xₖ in x))
_dot(x, y, ::UnconjugatedDot) = sum(prod, zip(x, y))
Copy link
Member

Choose a reason for hiding this comment

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

sqrt(sum(xₖ -> xₖ^2, x)) for the norm, so that it can use pairwise summation (iterators are not indexable)?

Copy link
Author

Choose a reason for hiding this comment

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

Wow, this change makes a significant difference in performance:

julia> x = rand(1000);

julia> @btime sum(xₖ^2 for xₖ in $x);
  1.221 μs (0 allocations: 0 bytes)

julia> @btime sum(xₖ->xₖ^2, $x);
  85.675 ns (0 allocations: 0 bytes)

But it doesn't seem like indexibility is the only condition for the pairwise summation to kick in. I tried to achieve a similar enhancement by pairwise summation in _dot(x, y, ::UnconjugatedDot) = sum(prod, zip(x, y)) by replacing zip(x,y) with an indexible custom type, but it didn't work as expected. Looked into the code in detail, and it seems that mapreduce_impl will need to be implemented for the custom type replacing zip(x,y)...

test/cg.jl Outdated
@test norm(A * x2 - b) / norm(b) ≤ reltol

# The following tests fails CI on Windows and Ubuntu due to a
# `SingularException(4)`
Copy link
Member

Choose a reason for hiding this comment

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

What's going on with this failure?

Copy link
Author

Choose a reason for hiding this comment

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

Actually the enclosing @testset of this @test is copied from L13-L43 of test/bicgstabl.jl. I didn't pay too much attention to the copied tests. Let me try to run the tests on a Windows box and get back to you.

Copy link
Author

Choose a reason for hiding this comment

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

I did pkg> test IterativeSolvers in a Windows box with the continue block (L35-L37 of test/bicgstabl.jl) commented out, and all the tests run fine. I will push the commit without the continue block in test/cg.jl. Let's see if the tests succeed...

Copy link
Author

Choose a reason for hiding this comment

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

No failure during CI (see below). I will probably open another PR to remove the continue block from test/bicgstabl.jl.

test/cg.jl Outdated
# Do an exact LU decomp of a nearby matrix
F = lu(A + rand(T, n, n))
x3, his3 = cocg(A, b, Pl = F, maxiter = 100, reltol = reltol, log = true)
@test norm(A * x3 - b) / norm(b) ≤ reltol
Copy link
Member

@stevengj stevengj Mar 16, 2021

Choose a reason for hiding this comment

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

For tests like this you can use . It should be equivalent to:

@test A*x3  b rtol=reltol

(see isapprox).

Copy link
Author

Choose a reason for hiding this comment

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

Hmmm. I cannot seem to specify the keyword arguments of isapprox in its infix form:

julia> VERSION
v"1.5.4-pre.0"

julia> 0 ≈ 0
true

julia> 0 ≈ 0, rtol=1e-8
ERROR: syntax: "0" is not a valid function argument name around REPL[23]:1

Is this a new capability introduced in version > 1.5.4?

Copy link
Member

Choose a reason for hiding this comment

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

Don't use the comma, and have it in a @test:

julia> @test 0  0 rtol=1e-8
Test Passed

src/cg.jl Outdated
cocg!(x, A, b; kwargs...) -> x, [history]

Same as [`cg!`](@ref), but uses the unconjugated dot product instead of the usual,
conjugated dot product.
Copy link
Member

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.

src/cg.jl Show resolved Hide resolved
Copy link
Member

@haampie haampie left a comment

Choose a reason for hiding this comment

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

Can you also list this method in the docs? https://julialinearalgebra.github.io/IterativeSolvers.jl/dev/linear_systems/cg/ maybe as part of this page?

src/cg.jl Outdated

Same as [`cg!`](@ref), but uses the unconjugated dot product (`xᵀy`) instead of the usual,
conjugated dot product (`x'y`) in the algorithm. It is for solving linear systems with
matrices `A` that are complex-symmetric (`Aᵀ == A`) rahter than Hermitian (`A' == A`).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
matrices `A` that are complex-symmetric (`Aᵀ == A`) rahter than Hermitian (`A' == A`).
matrices `A` that are complex-symmetric (`Aᵀ == A`) rather than Hermitian (`A' == A`).

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for finding out the typo! Will correct this in the next commit.

src/common.jl Outdated
_dot(x, y, ::ConjugatedDot) = dot(x, y)

_norm(x, ::UnconjugatedDot) = sqrt(sum(xₖ->xₖ^2, x))
_dot(x, y, ::UnconjugatedDot) = transpose(@view(x[:])) * @view(y[:]) # allocating, but faster than sum(prod, zip(x,y))
Copy link
Member

@haampie haampie Mar 22, 2021

Choose a reason for hiding this comment

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

Suggested change
_dot(x, y, ::UnconjugatedDot) = transpose(@view(x[:])) * @view(y[:]) # allocating, but faster than sum(prod, zip(x,y))
_dot(x, y, ::UnconjugatedDot) = transpose(x) * y

this shouldn't allocate, at least not on julia 1.5 and above; so let's drop that comment.

Copy link
Author

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 and y 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 for x and y 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 ...?

Copy link
Member

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 and y are assumed to behave as vectors; we have no block methods.

Copy link
Member

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

Copy link
Author

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 and y are assumed to behave as vectors; we have no block methods.

What if x and y are multi-dimensional arrays? It is quite common for iterative methods to handle such x and y with matrix-free operators A acting on them.

It is true that multi-dimensional arrays x and y are vector-like in a sense that vector operators such as dot(x, y) are defined on them. Other methods in IterativeMethods 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...

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

Fair enough. I will remove the comment in the next commit.

Copy link
Member

@haampie haampie Mar 23, 2021

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, not AX=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

struct MyTensorWithVectorSemantics{T} <: AbstractVector{T}
  X::Array{T, 3}
end

and define the interface for it.

Or you don't and you call cg(my_operator, reshape(B, :)) where my_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.

Copy link
Author

@wsshin wsshin Mar 27, 2021

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 current cg() implementation interprets a non-vector b (including b::AbstractMatrix) as a single right-hand-side column vector, as long as * is defined between the given A and b. I think supporting such usage in the high-level interface in IterativeSolvers.jl was the conclusion people reached after the lengthy discussion in #2 about duck-typing.

That said, my current proposal

_norm(x, ::UnconjugatedDot) = sqrt(sum(xₖ->xₖ^2, x))
_dot(x, y, ::UnconjugatedDot) = transpose(@view(x[:])) * @view(y[:])

is not the perfect solution that supports the most general type of x either: _norm assumes iterable x and _dot assumes reshapable x, and the Banach space element mentioned in #289 (comment) satisfies neither condition. The proposal just makes cocg() works for the most popular cases of x::AbstractArray without requiring the users to define _norm(x, ::UnconjugatedDot) and _dot(x, y, ::UnconjugatedDot) separately. For other types of x, 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 like Transpose or Adjoint in the standard library and rewrite LinearAlgebra:dot to interpret dot(Conjugate(x), y) as the unconjugated dot product.)

Copy link
Author

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?

@wsshin
Copy link
Author

wsshin commented Mar 22, 2021

Can you also list this method in the docs? https://julialinearalgebra.github.io/IterativeSolvers.jl/dev/linear_systems/cg/ maybe as part of this page?

So this is the part I was curious about. I haven't created docs for Julia packages before. Is there a quick guide for creating docs?

@haampie
Copy link
Member

haampie commented Mar 22, 2021

You can do

$ cd path/to/IterativeSolvers/docs
$ julia
julia> using Revise

(@v1.5) pkg> activate .
 Activating environment at `~/.julia/dev/IterativeSolvers/docs/Project.toml`

(docs) pkg> instantiate

julia> include("make.jl") # repeatedly

and edit docs/* and open build/index.html in your browser

@stevengj
Copy link
Member

stevengj commented Mar 22, 2021

@wsshin, see https://juliadocs.github.io/Documenter.jl/stable/ … you might add a note to https://github.com/JuliaLinearAlgebra/IterativeSolvers.jl/blob/master/docs/src/linear_systems/cg.md and https://github.com/JuliaLinearAlgebra/IterativeSolvers.jl/blob/master/docs/src/index.md

(The above commands are only only needed to build the doc pages locally; they will be built automatically when a PR is merged.)

@wsshin
Copy link
Author

wsshin commented Jan 5, 2022

@haampie, it took a while for me to add the documentation for COCG. Hopefully the latest commits are good to be merged. Note that CI tests are failing for Julia 1, but the failures are from gmres, not from cg or cocg.

@wsshin wsshin requested a review from haampie January 16, 2022 16:12
@wsshin wsshin requested a review from dkarrasch February 2, 2022 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants