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

Adding preconditioner to idrs #297

Merged
merged 3 commits into from
May 16, 2021
Merged

Adding preconditioner to idrs #297

merged 3 commits into from
May 16, 2021

Conversation

deltaeecs
Copy link
Contributor

Here I added Pl preconditioner to idrs add tested it with IncompleteLU preconditioner.

@mschauer
Copy link
Member

Nice! Did you follow literature or is it just clear how to add the preconditioner here? Pinging @astudillor, can you also have a cursory look?

s::Number, abstol::Real, reltol::Real, maxiter::Number; smoothing::Bool=false, verbose::Bool=false
) where {T}
s::Number, Pl::precT, abstol::Real, reltol::Real, maxiter::Number; smoothing::Bool=false, verbose::Bool=false
) where {T, precT}
Copy link
Member

Choose a reason for hiding this comment

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

I think you never use precT, so you don't need that type parameter in the method signature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I just followed the "gmres.jl" and added "precT". Maybe it will be useful in the future?

Project.toml Outdated
@@ -3,6 +3,7 @@ uuid = "42fd0dbc-a981-5370-80f2-aaf504508153"
version = "0.9.0"

[deps]
IncompleteLU = "40713840-3770-5561-ab4c-a76e7d0d7895"
Copy link
Member

Choose a reason for hiding this comment

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

We have a file for test dependencies, https://github.com/JuliaLinearAlgebra/IterativeSolvers.jl/blob/master/test/Project.toml, we only need that dependency for the test, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked the "gmres.jl" and found it use LU instead of the IncompleteLU I added in the dependency,shall I removed it?

Copy link
Member

Choose a reason for hiding this comment

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

It makes sense to have a non-exact preconditioner in the test, or? Perhaps just move the

IncompleteLU = "40713840-3770-5561-ab4c-a76e7d0d7895"

line to https://github.com/JuliaLinearAlgebra/IterativeSolvers.jl/blob/master/test/Project.toml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@deltaeecs
Copy link
Contributor Author

Nice! Did you follow literature or is it just clear how to add the preconditioner here? Pinging @astudillor, can you also have a cursory look?

hahaha
I followed the Matlab Code referred in the docs because I found idrs performs better than gmres in my work~

@mschauer
Copy link
Member

Haha, you can also compare with https://github.com/astudillor/idrs/blob/master/idrs/idrs.py

test/idrs.jl Outdated Show resolved Hide resolved
Co-authored-by: Moritz Schauer <[email protected]>
@deltaeecs
Copy link
Contributor Author

Haha, you can also compare with https://github.com/astudillor/idrs/blob/master/idrs/idrs.py

Thank you for your help~
I think they are the same after checked the idrs.py~
I'm so happy since it's my first time to do a little work on git~

@mschauer
Copy link
Member

Great. Can I ask on what type of problem you are working such that IDR(s) beats GMRES?

@deltaeecs
Copy link
Contributor Author

Great. Can I ask on what type of problem you are working such that IDR(s) beats GMRES?

It's a badly conditioned matrix equation on Computational Electromagneticcalled EFIE~

@coveralls
Copy link

Pull Request Test Coverage Report for Build 842243156

  • 0 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.2%) to 96.29%

Totals Coverage Status
Change from base Build 669197700: -1.2%
Covered Lines: 1713
Relevant Lines: 1779

💛 - Coveralls

@coveralls
Copy link

coveralls commented May 14, 2021

Pull Request Test Coverage Report for Build 842243156

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 97.776%

Totals Coverage Status
Change from base Build 669197700: 0.3%
Covered Lines: 1934
Relevant Lines: 1978

💛 - Coveralls

@deltaeecs
Copy link
Contributor Author

deltaeecs commented May 15, 2021

Great. Can I ask on what type of problem you are working such that IDR(s) beats GMRES?

It's a badly conditioned matrix equation on Computational Electromagneticcalled EFIE~

Well it seems to be a problem of problem of setting on preconditioner. GMRES remains the best solver after I corrected it.

@mschauer mschauer merged commit 281d229 into JuliaLinearAlgebra:master May 16, 2021
@mschauer
Copy link
Member

Ref #302

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.

3 participants