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 a keyword argument iostream for all Krylov methods #677

Merged
merged 5 commits into from
Nov 11, 2022
Merged

Conversation

amontoison
Copy link
Member

@amontoison amontoison commented Nov 8, 2022

https://juliasmoothoptimizers.github.io/Krylov.jl/previews/PR677/

We will be able to store the outputs of verbose mode with this modification.

@amontoison amontoison requested review from dpo and AntoninKns November 8, 2022 22:11
@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Package name latest stable
CaNNOLeS.jl
DCISolver.jl
FletcherPenaltySolver.jl
JSOSolvers.jl
LLSModels.jl
Percival.jl
RipQP.jl

@dpo
Copy link
Member

dpo commented Nov 9, 2022

Good idea! Next best thing to a logger!

@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Base: 98.19% // Head: 98.24% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (e8a45cb) compared to base (630faac).
Patch coverage: 91.22% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #677      +/-   ##
==========================================
+ Coverage   98.19%   98.24%   +0.04%     
==========================================
  Files          37       37              
  Lines        6601     6599       -2     
==========================================
+ Hits         6482     6483       +1     
+ Misses        119      116       -3     
Impacted Files Coverage Δ
src/krylov_utils.jl 92.55% <ø> (ø)
src/cr.jl 66.35% <31.03%> (ø)
src/bicgstab.jl 100.00% <100.00%> (ø)
src/bilq.jl 100.00% <100.00%> (ø)
src/bilqr.jl 100.00% <100.00%> (ø)
src/cg.jl 100.00% <100.00%> (ø)
src/cg_lanczos.jl 100.00% <100.00%> (ø)
src/cg_lanczos_shift.jl 100.00% <100.00%> (+2.54%) ⬆️
src/cgls.jl 100.00% <100.00%> (ø)
src/cgne.jl 100.00% <100.00%> (ø)
... and 24 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@amontoison
Copy link
Member Author

amontoison commented Nov 9, 2022

I don't understand why I have allocations when I use a sparse matrix and not when I use a dense one. 🤔

using Printf

function krylov(A, b::AbstractVector{T}; verbose::Int=0, iostream::IO=stdout) where {T}
  verbose > 0 && @printf(iostream, "%3d\n", 42)
  return nothing
end

A = rand(10, 10)
b = rand(10)
krylov(A, b)  # warm up
@allocated krylov(A, b)                   # 0 byte
krylov(A, b, iostream=stdout)  # warm up
@allocated krylov(A, b, iostream=stdout)  # 32 bytes

A2 = sprand(10, 10, 0.5)
b2 = rand(10)
krylov(A2, b2)  # warm up
@allocated krylov(A2, b2)                   # 48 bytes
krylov(A2, b2, iostream=stdout)  # warm up
@allocated krylov(A2, b2, iostream=stdout)  # 32 bytes

const kstdout = stdout

function krylov2(A, b::AbstractVector{T}; verbose::Int=0, iostream::IO=kstdout) where {T}
  verbose > 0 && @printf(iostream, "%3d\n", 42)
  return nothing
end

A = rand(10, 10)
b = rand(10)
krylov2(A, b)  # warm up
@allocated krylov2(A, b)                    # 0 byte
krylov2(A, b, iostream=kstdout)  # warm up
@allocated krylov2(A, b, iostream=kstdout)  # 16 bytes

A2 = sprand(10, 10, 0.5)
b2 = rand(10)
krylov2(A2, b2)  # warm up
@allocated krylov2(A2, b2)                    # 0 byte
krylov2(A2, b2, iostream=kstdout)  # warm up
@allocated krylov2(A2, b2, iostream=kstdout)  # 16 bytes

@dpo
Copy link
Member

dpo commented Nov 9, 2022

What does the @code_warntype say? If nothing useful, you may have to look at the low-level translation.

@amontoison
Copy link
Member Author

amontoison commented Nov 9, 2022

What does the @code_warntype say? If nothing useful, you may have to look at the low-level translation.

I checked with @code_warntype and it says nothing special.
We have some small differences with @code_llvmbut I don't understand them.

I will open an issue in the GitHub repository of Julia.

@amontoison
Copy link
Member Author

amontoison commented Nov 9, 2022

@dpo
I found the problem, stdout from Base is a global variable and its type is not fixed.
It means that the compiler can only determine the type at runtime.
The type of stdout is different if we use redirect_stdout(mystream) for instance.

I don't have allocations with Core.stdout because its type can't be modified.
I propose to use this one by default and use kstdout as alias.

@dpo
Copy link
Member

dpo commented Nov 9, 2022

great that it was not a bug!

@amontoison amontoison force-pushed the iostream branch 2 times, most recently from 358ee40 to afc70af Compare November 10, 2022 16:04
@amontoison amontoison merged commit c98beb4 into main Nov 11, 2022
@amontoison amontoison deleted the iostream branch November 11, 2022 21:17
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