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

Migration to IterativeSolvers, bug fixes, increased coverage #65

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SebastianAment
Copy link
Contributor

Iterative Solvers

The main goal of this PR is to use the iterative solver routines from IterativeSolvers.jl. To this end, new features include:

  • Solves with rectangular systems via lsqr.
  • Solves with indefinite symmetric systems via gmres (the current release of ToeplitzMatrices.jl assumes that all symmetric systems are positive definite, and uses cg).
  • Deletion of the iterativeLinearSolvers.jl file

New Functionality

Beside this high level goal, the PR also defines several new functions for Toeplitz matrices and factorizations:

  • inv for Toeplitz matrices
  • adjoint, size, and copy for ToeplitzFactorization structures
  • apply(f, C) for Circulant and CirculantFactorization structures C, which applies f to the eigenvalues of C, in this way defining the matrix functions abs, sqrt, and inv.

Testing Coverage

Further, this PR includes a testing suite with greater coverage.

Bug Fix

In writing the new tests, it became clear that inv(::TriangularToeplitz) was broken on Julia 1.7 (and presumable several previous versions too, since nextpow2 and conv haven't been in Base for a while). This PR also fixes the function.

@dlfivefifty
Copy link
Member

@marcusdavidwebb @ajt60gaibb any opinions on this?

@codecov
Copy link

codecov bot commented Jan 6, 2022

Codecov Report

Merging #65 (e220d55) into master (edbc8eb) will increase coverage by 9.30%.
The diff coverage is 94.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage   76.60%   85.90%   +9.30%     
==========================================
  Files           2        1       -1     
  Lines         483      447      -36     
==========================================
+ Hits          370      384      +14     
+ Misses        113       63      -50     
Impacted Files Coverage Δ
src/ToeplitzMatrices.jl 85.90% <94.78%> (+12.55%) ⬆️

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 edbc8eb...e220d55. Read the comment docs.

@coveralls
Copy link

coveralls commented Jan 6, 2022

Coverage Status

Coverage increased (+9.3%) to 85.906% when pulling e220d55 on SebastianAment:iterative-linear-solvers into edbc8eb on JuliaMatrices:master.

@dlfivefifty
Copy link
Member

We can drop Julia v1.0 support and just require Julia v1.6 if you bump the minor version

@SebastianAment
Copy link
Contributor Author

I adjusted the Julia compatibility entry, the CI file, and bumped the minor version. Let me know if there's anything else that needs modification.

@SebastianAment
Copy link
Contributor Author

There was one test that failed in one out of the six CI runs. I reduced the accuracy requirement of the test to the accuracy requirement of the iterative solver, which should make the test pass.

@renespoerer
Copy link

I'm interested in the features introduced in this pull request and noticed it has been inactive for nearly two years. Specifically, I'm interested in using gmres from IterativeSolvers.jl on ToeplitzFactorization structures.

Could the maintainers shed light on the status of this PR? Are there unresolved issues or specific reasons preventing its integration? I'm keen to assist, be it resolving merge conflicts or updating the code, to facilitate its inclusion.

@dlfivefifty
Copy link
Member

  1. Probably no one got around to merging it.
  2. Now there are a lot of conflicts.
  3. Skimming through, it should be change to an extension, not adding IterativeSolvers.jl as a dependency. That extension can live here or in IterativeSolvers.jl

@renespoerer
Copy link

Thank you for the response, @dlfivefifty! I'll have a look at this in the next weeks.

renespoerer added a commit to renespoerer/ToeplitzMatrices.jl that referenced this pull request Dec 15, 2023
Added adjoint, copy, size, and length for ToeplitzFactorization; added
fields n and m to facilitate this.

Copied from PR JuliaLinearAlgebra/pull/65

Co-authored-by: Sebastian Ament <sebastianeament@gmail.com>
renespoerer added a commit to renespoerer/ToeplitzMatrices.jl that referenced this pull request Dec 15, 2023
Added adjoint, copy, size, and length for ToeplitzFactorization; added
fields n and m to facilitate this.

Copied from PR JuliaLinearAlgebra/pull/65 by
Sebastian Ament <sebastianeament@gmail.com>.
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.

None yet

4 participants