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 Chbv #16

Merged
merged 1 commit into from
Oct 28, 2017
Merged

Add Chbv #16

merged 1 commit into from
Oct 28, 2017

Conversation

mforets
Copy link
Contributor

@mforets mforets commented Oct 21, 2017

No description provided.

@mforets mforets mentioned this pull request Oct 21, 2017
@mforets
Copy link
Contributor Author

mforets commented Oct 23, 2017

in commit bfe6cee, the inner accumulations are done inplace. notice that the auxiliary vector y should be complex in both cases.

do we want to have an inplace version similar to expmv!, like padm!{T}(w::Vector{T}, A, v::Vector{T})?

src/chbv.jl Outdated
error("dimension mismatch")
end

p = 7
Copy link
Owner

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 p=length(α) (or even min(length(α),length(θ))) to make sure that we stay in the bounds. One could also put θ and α into one matrix ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, changed.

src/chbv.jl Outdated
y = α0 * vec
if (isreal(vec) && isreal(A))
@inbounds for i = 1:p
y .+= (A - θ[i]*I) \ (α[i] * vec)
Copy link
Owner

Choose a reason for hiding this comment

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

If one wants to keep y real, one could just sum the real parts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thanks for the observation; this allows me to get rid of w.

src/chbv.jl Outdated

p = 7

I = eye(A)
Copy link
Owner

Choose a reason for hiding this comment

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

I think we don't need to construct an explicit identity matrix, we can rely on UniformScaling (I) below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, this saves some allocation, changed.

@mforets
Copy link
Contributor Author

mforets commented Oct 27, 2017

FYI for this function i collected some benchmark data, it's here.

Now i've started to do the chbv! version, locally, but i get some errors.

src/chbv.jl Outdated
y .+= real((A - θ[i]*I) \ (α[i] * vec))
end
else
t = [θ; conj(θ)]
Copy link
Owner

Choose a reason for hiding this comment

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

I was wondering if we should add those by hand to the definitions of θ and α above. It is not saving a lot of memory, but will reduce garbage collection if called frequently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean to define a table for theta_conj and alpha_conj as well, right? yes, we can add those defs on top.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, or you can just add it to the vectors you already have (but then p=length(α)/2).

@mforets
Copy link
Contributor Author

mforets commented Oct 27, 2017

i'm struggling with the inplace version, let me put the broken version here 🤕

actually if i use the three input inplace versions, chbv!(Hv, H, v), work properly, but the two-component one is wrong.

function chbv{T}(A, vec::Vector{T})
    result = convert(Vector{promote_type(eltype(A), T)}, copy(vec))
    chbv!(A, result)
    return result
end

# wrong, use: chbv!(H, v);
chbv!{T}(A, vec::Vector{T}) = chbv!(vec, A, vec)

# OK, use: Hv = similar(v); chbv!(Hv, H, v); norm(Hv - expm(H) * v) ~= 1e-12
function chbv!{T<:Real}(w::Vector{T}, A, vec::Vector{T})
    p = min(length(θ), length(α))
    scale!(copy!(w, vec), α0)

    @inbounds for i = 1:p
        w .+= real((A - θ[i]*I) \ (α[i] * vec))
    end
    return w
end

# OK, use: Hv = similar(v); chbv!(Hv, H, v); norm(Hv - expm(H) * v) ~= 1e-12
function chbv!{T<:Complex}(w::Vector{T}, A, vec::Vector{T})
    p = min(length(θ), length(α))
    scale!(copy!(w, vec), α0)
    t = [θ; conj(θ)]
    a = 0.5 * [α; conj(α)]

    @inbounds for i = 1:2*p
        w .+= (A - t[i]*I) \ (a[i] * vec)
    end
    return w
end

EDIT: some comments and removed vec=w in the end of each function.

@acroy
Copy link
Owner

acroy commented Oct 27, 2017

The two argument version of chbv! cannot work since you overwrite the input vec which you need further down. I think you have to do chbv!{T}(A, vec::Vector{T}) = chbv!(similar(vec), A, vec) to make it work (or even using similar(vec, promote_type(eltype(A), T))).

@mforets
Copy link
Contributor Author

mforets commented Oct 27, 2017

but the vec doesn't get updated: chbv!(H, v) returns the correct value, although it is not attached to v after the call.

@acroy
Copy link
Owner

acroy commented Oct 27, 2017

True, then maybe chbv!{T}(A, vec::Vector{T}) = chbv!(vec, A, copy(vec)) will do?

@mforets
Copy link
Contributor Author

mforets commented Oct 27, 2017

Well done! That does it. I've updated the branch.

src/chbv.jl Outdated
function chbv(A, vec)
function chbv{T}(A, vec::Vector{T})
result = convert(Vector{promote_type(eltype(A), T)}, copy(vec))
chbv!(A, result)
Copy link
Owner

Choose a reason for hiding this comment

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

I think this will result in an unnecessary copy. It's better to use the three argument version here.

@acroy
Copy link
Owner

acroy commented Oct 28, 2017

Do you have an idea why the tests for 0.4 fail? I will try to test locally ...

@mforets
Copy link
Contributor Author

mforets commented Oct 28, 2017

No idea. I'm in v0.6 and for the failing test in v0.4 i get that the residuum is smaller than 1e-11 for a set of 10.000 tests.

I saw that JuliaBox proposes older kernels too, i can check there..

@mforets
Copy link
Contributor Author

mforets commented Oct 28, 2017

i can confirm that there is an issue with v0.4.7.

with the 3 argument variant, it does produce the correct result, but it doesn't modify its first argument when the function is returned. like a different handling of variables scope?

@mforets
Copy link
Contributor Author

mforets commented Oct 28, 2017

to be precise, this happens:

function f(w)
    w .+= ones(length(w))
    return w
end
w = 2.0 * ones(4);
f(w), w
# ([3.0,3.0,3.0,3.0],[2.0,2.0,2.0,2.0]) in v0.4.7
# ([3.0, 3.0, 3.0, 3.0], [3.0, 3.0, 3.0, 3.0]) in v0.5.1 and later

i dunno if Compat is supposed to "fix this".

@acroy
Copy link
Owner

acroy commented Oct 28, 2017

Ah, good catch! This is probably because .+= was not in place pre 0.5. We should be able to "fix" this by returning chbv!(result, A, vec) instead of result. At least the tests will pass.

@mforets
Copy link
Contributor Author

mforets commented Oct 28, 2017

Done. Shall i squash commits into a single one?

@acroy
Copy link
Owner

acroy commented Oct 28, 2017

Yes, that would be great. We will also have to add the information to the README file, or do you want to generate that from the docstrings?

@mforets
Copy link
Contributor Author

mforets commented Oct 28, 2017

ok, done.

regarding documentation, yes, in case you haven't tried it yet with Documenter.jl the setup is super easy and plays very well with github. kudos to Documenter devs :)
in this case we can enclose the function's name in a @docs macro, in a file that can be e.g. docs/src/lib/chbv.md.

@acroy acroy merged commit d18e3b7 into acroy:master Oct 28, 2017
@acroy
Copy link
Owner

acroy commented Oct 28, 2017

Cool. I had not seen Documenter.jl, but it looks really nice and useful!

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.

2 participants