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

Remove FactCheck, use Base.test and simplify tests #150

Merged
merged 40 commits into from Aug 7, 2017

Conversation

@haampie
Copy link
Collaborator

haampie commented Jul 25, 2017

This touches all tests, so a bit of a scary PR.

It removes FactCheck and moves to Julia's native Base.Test

It also simplifies a lot of tests and removes some warnings in 0.6 about broadcasting.

With regard to the lsmr.jl test, it's quite elaborate (the types & methods in the test itself almost need tests), but the actual thing that is being tested is whether or not lsmr support custom matrix and vector types. I think checking for custom matrix types can just as well be done by wrapping a matrix in a LinearMap, rather than writing hard to follow implicit matrix-vector product routines.

Concerning the custom vector types: that's something we need to test not just for lsmr, but for all methods. For instance using DistributedVector.

This PR also removes the test with the test/IterativeSolvers.jld which is basically a huge matrix (see #117).

Lastly, the tests in history.jl should probably not be smoke tests like they are now. I think it makes more sense to just unit test a ConvergenceHistory object.

@haampie haampie force-pushed the haampie:fix-tests branch from 0e858c6 to d244869 Jul 26, 2017
@haampie
Copy link
Collaborator Author

haampie commented Jul 26, 2017

It turns out I accidentally broke support for Julia 0.5 in multiple ways:

  • The new version of LinearMaps.jl drops 0.6 support
  • @test [expr] [kwargs] syntax is not backwards compatible
  • The GMRES test with preconditioners and sparse matrices was broken, and after a fix does not pass on Julia 0.5. It accidentally converted sparse to dense via sprand(...) + eye(...), which I have fixed to sprand(...) + speye(...). And only then the tests failed on Julia 0.5 because A_ldiv_B!(::UmfpackLU, ::SubArray) was not implemented. Current solution is to do the test conditionally on the version number.
@haampie haampie force-pushed the haampie:fix-tests branch from d244869 to 4e3edbd Jul 26, 2017
b += im*randn(n)
end
@testset "SparseMatrixCSC{$T}" for T in (Float64, Complex128)
A = sprand(T, n, n, 0.5) + speye(T, n, n)

This comment has been minimized.

@andreasnoack

andreasnoack Jul 26, 2017 Member

Use I here instead of speye

This comment has been minimized.

@haampie

haampie Jul 26, 2017 Author Collaborator

Nice, I'll use that throughout the code then

@haampie haampie force-pushed the haampie:fix-tests branch from 4e3edbd to 9f76e69 Jul 26, 2017
@haampie haampie force-pushed the haampie:fix-tests branch from e047c74 to bf7b454 Jul 27, 2017
Copy link
Member

andreasnoack left a comment

Two minor questions. Otherwise, I think it is good togo.

src/cg.jl Outdated
# Return the iterable
if isa(Pl, Identity)
return CGIterable(A, x, b,
r, c, u,
reltol, residual, ρ,
reltol, residual, residual,

This comment has been minimized.

@andreasnoack

andreasnoack Aug 3, 2017 Member

This seems to change the test. It used to be one, right? Was it on purpose.

This comment has been minimized.

@haampie

haampie Aug 4, 2017 Author Collaborator

Whoops, yeah, that seems to be an error. I'll fix it

context("Sparse Laplacian") do
A = poisson_matrix(Float64, 32, 3)
@testset "Sparse Laplacian" begin
A = poisson_matrix(Float64, 10, 3)

This comment has been minimized.

@andreasnoack

andreasnoack Aug 3, 2017 Member

Why change the size?

This comment has been minimized.

@haampie

haampie Aug 4, 2017 Author Collaborator

At the moment is generates a matrix of size 32^3 = 32.768, which is rather large for a test.

@andreasnoack andreasnoack merged commit 27b87a9 into JuliaMath:master Aug 7, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.6%) to 87.782%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.