JuliaLinearAlgebra / IterativeSolvers.jl

Iterative algorithms for solving linear systems, eigensystems, and singular value problems
MIT License
399 stars 105 forks source link

adding type to reserve! in most solvers #263

Open nestordemeure opened 4 years ago

nestordemeure commented 4 years ago

When storing a log, solvers reserve memory with reserve!. By default no type information is passed to this method which, then, converts the history into Float64. This is not the expected behaviour when using non traditional types (for instrumentation purpose) and can lead to problems if their silent convertion into Float64 is not detected by the programmer.

Thus I replaced several occurences of reserve!(history, ... with reserve!(typeof(tol), history, ....

I did not modify lsqr.jl, lsmr.jl and svdl.jl as the modification was less straightforward but they should probably be modified in the same way.

(overall I think that reserve! should always stipulate the expected type to avoid regression on this issue)

codecov-io commented 4 years ago

Codecov Report

Merging #263 into master will decrease coverage by 0.05%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #263      +/-   ##
==========================================
- Coverage   90.61%   90.56%   -0.06%     
==========================================
  Files          18       18              
  Lines        1130     1124       -6     
==========================================
- Hits         1024     1018       -6     
  Misses        106      106
Impacted Files Coverage Δ
src/qmr.jl 90.56% <100%> (ø) :arrow_up:
src/minres.jl 95.74% <100%> (ø) :arrow_up:
src/bicgstabl.jl 94.73% <100%> (ø) :arrow_up:
src/simple.jl 85.71% <100%> (ø) :arrow_up:
src/chebyshev.jl 93.75% <100%> (ø) :arrow_up:
src/cg.jl 95% <100%> (-0.35%) :arrow_down:
src/idrs.jl 91.93% <100%> (ø) :arrow_up:
src/gmres.jl 93.22% <100%> (ø) :arrow_up:
src/lsqr.jl 97.5% <0%> (-0.18%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e090fc6...59c25db. Read the comment docs.

mschauer commented 4 years ago

Out of curiosity, how did you run into this?

nestordemeure commented 4 years ago

I did tests with an instrumented type that does not implement silent conversion to Float64 to avoid bugs due to unseen conversions.

ranocha commented 3 years ago

Bump. This looks like a fix that would be nice to have in IterativeSolvers.jl.

ranocha commented 3 years ago

What do you think of this, @haampie?