JuliaIntervals / IntervalRootFinding.jl

Library for finding the roots of a function using interval arithmetic
https://juliaintervals.github.io/IntervalRootFinding.jl/
Other
127 stars 26 forks source link

System of Linear Equations #67

Closed eeshan9815 closed 6 years ago

codecov-io commented 6 years ago

Codecov Report

Merging #67 into master will increase coverage by 6.37%. The diff coverage is 85.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
+ Coverage   56.14%   62.52%   +6.37%     
==========================================
  Files          10       10              
  Lines         431      467      +36     
==========================================
+ Hits          242      292      +50     
+ Misses        189      175      -14
Impacted Files Coverage Δ
src/IntervalRootFinding.jl 4.65% <ø> (+2.61%) :arrow_up:
src/linear_eq.jl 85.13% <85.13%> (ø)
src/contractors.jl 90% <0%> (-0.48%) :arrow_down:
src/newton.jl 0% <0%> (ø) :arrow_up:
src/krawczyk.jl 0% <0%> (ø) :arrow_up:
src/bisect.jl
src/roots.jl 88.13% <0%> (+2.19%) :arrow_up:
src/complex.jl 83.33% <0%> (+8.33%) :arrow_up:

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 25ebe4c...7746380. Read the comment docs.

dpsanders commented 6 years ago

Thanks! I don't see the benchmarks though.

eeshan9815 commented 6 years ago

I manually benchmarked, and shared the results on #intervals in slack

dpsanders commented 6 years ago

Please include at least the benchmarking script (using BenchmarkTools) in a new directory perf.

eeshan9815 commented 6 years ago

I have included the script and results for n = 1:25 cc @dpsanders

dpsanders commented 6 years ago

That's the kind of thing I had in mind, thanks. You could instead collect the data using @belapsed (I think) and output a table using Dataframes or something like that.

eeshan9815 commented 6 years ago

Okay, I'll try it out! So which version of the function should I retain? I don't think MArray improved the performance a lot.

dpsanders commented 6 years ago

Try à version using IntervalBox and setindex.

dpsanders commented 6 years ago

IntervalBox is immutable like an SVector. So modifying it actually requires making a new copy and changing the relevant entry using setindex (without !)

dpsanders commented 6 years ago

This needs tests.

eeshan9815 commented 6 years ago

@dpsanders Travis and AppVeyor both use the latest tagged version and don't have extended_div, and hence are failing.

lbenet commented 6 years ago

I tried to tag a new version but it was causing failures in ValidatedNumerics. I did some upgrades, but I think we should plan carefully how to make patches-versions, so they don't break everything. See this comment.

dpsanders commented 6 years ago

Actually the matrix version could be pretty efficient when using SMatrix and SVector everywhere. Have you tried that?

eeshan9815 commented 6 years ago

Done. I think this version gives the best benchmarks.

dpsanders commented 6 years ago

So it seems just using normal arrays is fine?

eeshan9815 commented 6 years ago

So it seems just using normal arrays is fine?

More or less, yes. Should I remove the other implementations and update the PR?

dpsanders commented 6 years ago

You should squash down the implementations as I suggested; I think you should leave all of them for now.

Could you please add the example from the Jaulin book in the examples folder?

Actually you should keep a separate repo, e.g. in your GitHub account or in JuliaIntervals, with a Jupyter notebook with usage examples and explanation.

eeshan9815 commented 6 years ago

@dpsanders Is this accurate?

eeshan9815 commented 6 years ago

@dpsanders I've added the gaussian elimination with preconditioning and benchmarking against Base.\

eeshan9815 commented 6 years ago

ping @dpsanders

dpsanders commented 6 years ago

This and your other PRs need rebasing on current master, sorry!

eeshan9815 commented 6 years ago

Done rebasing and tests pass on the current IntervalArithmetic master

dpsanders commented 6 years ago

There is still (or again) a conflict in test/runtests.jl.

dpsanders commented 6 years ago

(sorry, wrong button)

eeshan9815 commented 6 years ago

Oh that occurred due to the merging of #70. I have rebased again.

dpsanders commented 6 years ago

@eeshan9815 Tests are still failing. Do you know what is happening?

eeshan9815 commented 6 years ago

Tests fail as it uses extended_div which is not in the latest tagged version of IntervalArithmetic. Tests pass locally if the current master is used.

dpsanders commented 6 years ago

Ah great, thanks

eeshan9815 commented 6 years ago

I have redefined \ for Intervals. Currently, it uses the gauss_elimination_interval method with precondition=true. This is due to the advantages of preconditioning such as reducing dependence mentioned in various books such as Global Optimisation using Interval Analysis by Eldon Hansen.

eeshan9815 commented 6 years ago

Since \ is same as gauss_elimination_interval, for which tests exist already, is there a need to define tests for \ separately? Also, comparisions exist in the perf/linear_eq_tabular.txt file.

dpsanders commented 6 years ago

Right, but the tests are also for future-proofing, eg if we change the implementation of \ again. So you can just add a couple of tests to make sure the output of \ and gaussian elim are the same.

dpsanders commented 6 years ago

@eeshan9815 Did you actually check that the definition of \ in the package is being called? It doesn't seem to be. You need to import Base: \.

dpsanders commented 6 years ago

On the example in the testset "Linear Equations", the new definition seems to be worse than using Base's \.

eeshan9815 commented 6 years ago

I have imported Base.\ in IntervalRootFinding.jland it seems to be using gauss_elimination_interval instead of Base.\ Also, preconditioning widens the solution set by a little bit. It's a trade-off mentioned in the Hansen book as well.

dpsanders commented 6 years ago

Hmm, I see, sorry. I don't know why it didn't work for me, then.

What's the advantage of preconditioning, then? I thought it was supposed to narrow, not widen, the result.

dpsanders commented 6 years ago

After checking out your PR, I get

julia> A = [4..5 -1..1 1.5..2.5; -0.5..0.5 -7.. -5 1..2; -1.5.. -0.5 -0.7.. -0.5 2..3]
3×3 Array{IntervalArithmetic.Interval{Float64},2}:
 [4, 5]            [-1, 1]                    [1.5, 2.5]
    [-0.5, 0.5]   [-7, -5]                [1, 2]
    [-1.5, -0.5]       [-0.700001, -0.5]  [2, 3]

julia> b = [3..4, 0..2, 3..4]
3-element Array{IntervalArithmetic.Interval{Float64},1}:
 [3, 4]
 [0, 2]
 [3, 4]

julia> A \ b
3-element Array{IntervalArithmetic.Interval{Float64},1}:
 [-1.81928, 1.16873]
 [-0.41407, 1.72523]
  [0.700232, 3.42076]

julia> @which A \ b
\(A::AbstractArray{T,2} where T, B::Union{AbstractArray{T,1}, AbstractArray{T,2}} where T) in Base.LinAlg at linalg/generic.jl:820

which shows that the \ from Base is being used. Not sure why.

dpsanders commented 6 years ago

Ignore me, I seem not to have managed to checkout the latest version, apologies.

dpsanders commented 6 years ago

No, don't ignore me -- you defined \ in test/linear_eq.jl instead of in the source code!

dpsanders commented 6 years ago

No problem, thanks!

dpsanders commented 6 years ago

Running gauss_seidel seems to modify something it shouldn't? The result of A \ b shouldn't change in the following session:

julia> IntervalRootFinding.gauss_elimination_interval(A, b)
2-element Array{IntervalArithmetic.Interval{Float64},1}:
 [-130.228, 167.728]
  [-60.0001, 267.273]

julia> IntervalRootFinding.gauss_elimination_interval(A, b, precondition=false)
2-element Array{IntervalArithmetic.Interval{Float64},1}:
 [-120, 90]
 [-60, 240]

julia> IntervalRootFinding.gauss_seidel_interval(A, b, precondition=false)
2-element Array{IntervalArithmetic.Interval{Float64},1}:
 [-5e+15, 5.00001e+15]
 [-1e+16, 1e+16]

julia> A
2×2 Array{IntervalArithmetic.Interval{Float64},2}:
 [2, 3]  [0, 1]
 [1, 2]  [1, 3]

julia> b
2-element Array{IntervalArithmetic.Interval{Float64},1}:
   [0, 120]
 [-60, 240]

julia> A \ b
2-element Array{IntervalArithmetic.Interval{Float64},1}:
 [-1e+16, 1e+16]
 [-1e+16, 1e+16]
dpsanders commented 6 years ago

I think this needs more tests. E.g. the gauss_seidel methods give huge pointless answers for the single test case.

dpsanders commented 6 years ago

@eeshan9815 There seems to be a real test failure here.

eeshan9815 commented 6 years ago

The test failure appears to be in a test for 2D roots, giving error messages similar to the ones faced when the SVector was initially made a data member of IntervalBox. It should work after merging #83

eeshan9815 commented 6 years ago

The incorrect modification issue has also been resolved.

julia> IntervalRootFinding.gauss_elimination_interval(A, b)
2-element Array{IntervalArithmetic.Interval{Float64},1}:
 [-130.228, 167.728] 
  [-60.0001, 267.273]

julia> IntervalRootFinding.gauss_elimination_interval(A, b, precondition=false)
2-element Array{IntervalArithmetic.Interval{Float64},1}:
 [-120, 90]
 [-60, 240]

julia> IntervalRootFinding.gauss_seidel_interval(A, b, precondition=false)
2-element Array{IntervalArithmetic.Interval{Float64},1}:
 [-120.001, 90.0001] 
  [-60.0001, 240.001]

julia> A
2×2 Array{IntervalArithmetic.Interval{Float64},2}:
 [2, 3]  [0, 1]
 [1, 2]  [2, 3]

julia> b
2-element Array{IntervalArithmetic.Interval{Float64},1}:
  [0, 120]
 [60, 240]

julia> A \ b
2-element Array{IntervalArithmetic.Interval{Float64},1}:
 [-130.228, 167.728] 
  [-60.0001, 267.273]
dpsanders commented 6 years ago

@eeshan9815 Tests are now passing. Is this ready?

eeshan9815 commented 6 years ago

Yes, I think so. I added 88 tests (mostly randomly generated) to make sure the methods are working.

dpsanders commented 6 years ago

Thanks very much!