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

Julia 0.7 update #205

Merged
merged 11 commits into from
Jul 11, 2018
Merged

Conversation

lostella
Copy link
Contributor

@lostella lostella commented Jul 7, 2018

Addressing #191. This is an updated re-issuing of #198, taking into account all recent contributions to IterativeSolvers and changes to Julia 0.7. It relies on this PR for LinearMaps, so CI will fail before that is merged.

Open issues:

  • 3 occurrences of spdiagm (1 in benchmark, 2 in test) still need be updated and raise warnings as of now (I'm looking into this now).
  • I temporarily commented "Dampened tests" for LSMR, apparently the utility code for DampenedVector and DampenedMatrix needs to be extended to have this test working; I'm happy to fix that with some help from the authors.
  • Some tests (lobpcg.jl and stationary.jl) worked after changing the rng seed, otherwise some assertions complained about tolerances not being met (by a small margin) or matrices not being positive semi-definite.
    • stationary.jl
    • lobpcg.jl

I'd like to get inputs especially on the latter two items, as well as on the rest of the code of course.
For the rest, this PR should be merged as soon as this PR for LinearMaps is.

Edit: since this PR is 0.6-incompatible, I would also immediately

  • Update iterables interface.

@lostella lostella mentioned this pull request Jul 7, 2018
@lostella
Copy link
Contributor Author

lostella commented Jul 7, 2018

This is also the place where to get rid of #201, but after a quick check IterativeSolvers doesn't seem to be needing opnorm. Am I missing something?

@dkarrasch
Copy link
Member

Regarding the norm issue, a quick check indeed seems to say that only norms of vectors are computed, which is to be expected in the context of iterative solvers. At times, however, this is done via vecnorm (I checked the current version, not the PR), which is deprecated in favor of norm.

@mohamed82008
Copy link
Member

Tolerances for lobpcg were a bit funny before, often working on my machine but not on Travis, so I imagine the residual norm is sensitive to the machine code and possibly also the seed. I will take a closer look. Thanks for this btw.

@lostella
Copy link
Contributor Author

lostella commented Jul 9, 2018

I added somewhat minimal changes to make the package compliant with the new iteration protocol. These can easily be extended by completely getting rid of start and done.

Further restructuring & uniformization of the iterators I would defer to a later stage.

@haampie
Copy link
Member

haampie commented Jul 9, 2018

Thanks, this looks great, I'll look into that issue with stationary.jl. The DampenedVector stuff is imho a bit too much for a test, but let's see if it can be upgraded easily as well.

Edit, w.r.t stationary.jl: there is no stopping criterion based on the residual or something, so the test was a bit shaky. Current fix LGTM.

@haampie
Copy link
Member

haampie commented Jul 9, 2018

Hm, I think I'll just simplify the DampenedVector bit, cause as far as I see the only special thing is the DampenedMatrix.

… that is not in LinearAlgebra for dense matrices.
@lostella
Copy link
Contributor Author

lostella commented Jul 9, 2018

Yeah it definitely looks like there's something fishy with test tolerances in lobpcg.jl.

src/lobpcg.jl Outdated
@@ -869,9 +872,9 @@ function lobpcg!(iterator::LOBPCGIterator; log=false, tol=nothing, maxiter=200,
end
n = size(X, 1)
sizeX = size(X, 2)
residualTolerance = (tol isa Void) ? (eps(real(T)))^(real(T)(4)/10) : real(tol)
residualTolerance = (tol isa Nothing) ? (eps(real(T)))^(real(T)(4)/10) : real(tol)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could the problem be here? @mohamed82008
Tests may fail since

(eps(real(T)))^(real(T)(4)/10) > sqrt(eps(real(T)))

but the test tolerance is set to sqrt(eps(real(T))).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, tol is actually not a Nothing in tests: it is passed in and matches the test tolerance.

Copy link
Member

Choose a reason for hiding this comment

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

LOBPCG tests pass on my machine.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see you changed the seed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also runs fine on my machine (yes, with a modified seed), but apparently not on Travis.

@mohamed82008
Copy link
Member

All good now. It seems like LOBPCG works, but some other tests fail on my machine and there are other warnings. I will try to fix the warnings.

@lostella
Copy link
Contributor Author

Yep, thank you @mohamed82008! However, I don't see any warning on macos, and Travis doesn't show any on Linux.

Coverage decrease may be due to the @test_skipped tests: these will be re-enabled as soon as the PR for LinearMaps is merged (& version-bumped?)

@mohamed82008
Copy link
Member

mohamed82008 commented Jul 10, 2018

Yes the warnings are gone when I went to nightly, seems like it was some bug in the early v0.7 beta. Looking good!

Test Summary:    | Pass  Total
Basic operations |    4      4
Test Summary:     | Pass  Total
Orthogonalization |   24     24
Test Summary: | Pass  Total
Hessenberg    |    4      4
Test Summary:      | Pass  Total
Stationary solvers |   89     89
Test Summary:       | Pass  Total
Conjugate Gradients |   39     39
Test Summary: | Pass  Total
BiCGStab(l)   |   48     48
Test Summary: | Pass  Total
MINRES        |   36     36
Test Summary: | Pass  Total
GMRES         |   35     35
Test Summary: | Pass  Total
IDR(s)        |   26     26
Test Summary: | Pass  Total
Chebyshev     |   36     36
Test Summary:       | Pass  Total
Simple Eigensolvers |   24     24
Test Summary:                                           | Pass  Total
Locally Optimal Block Preconditioned Conjugate Gradient |  322    322
Test Summary: | Pass  Total
SVD Lanczos   |   32     32
Test Summary:         | Pass  Total
BrokenArrowBidiagonal |    8      8
Test Summary: | Pass  Total
LSQR          |   11     11
Test Summary: | Pass  Total
LSMR          |    9      9
Test Summary:      | Pass  Total
ConvergenceHistory |   17     17
   Testing IterativeSolvers tests passed

@haampie
Copy link
Member

haampie commented Jul 10, 2018

LGTM. I'll merge tomorrow and tag a new version :). Thanks very much @lostella!

@haampie haampie merged commit 781bcef into JuliaLinearAlgebra:master Jul 11, 2018
@clason
Copy link

clason commented Jul 11, 2018

Out of interest: I notice that https://github.com/JuliaRegistries/Uncurated / https://github.com/JuliaLang/METADATA.jl aren't actually picking up the new version (nor 0.6.0) -- is it by design that these two versions aren't tagged and published there (yet)?

@haampie
Copy link
Member

haampie commented Jul 11, 2018

@clason thanks for reminding me; there was an issue with the tests when tagging IterativeSolvers.jl on METADATA.jl. @mohamed82008 http://juliarun-ci.s3.amazonaws.com/1789ca0b09724ec0b42d7f890223157a2e4900eb/pull_request_of_IterativeSolvers_on_julia_0_6.log this is probably resolved with the fixes of this PR? If so I'll check if I can redo the 0.6.0-tag with these changes and then publish again.

@mohamed82008
Copy link
Member

I believe this should be fixed, but Travis can be surprising in bad ways sometimes.

@lostella lostella deleted the julia-0.7-update branch May 17, 2020 13:35
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.

5 participants