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

Unifying the interface options #279

Open
ranocha opened this issue Dec 4, 2020 · 5 comments
Open

Unifying the interface options #279

ranocha opened this issue Dec 4, 2020 · 5 comments

Comments

@ranocha
Copy link
Member

ranocha commented Dec 4, 2020

We have some applications where it would be very useful to have both absolute and relative tolerances, e.g. because we will run a lot of linear solves with good initial guesses such as in Newton methods. Currently, the linear solvers seem to support only relative tolerance (via the keyword argument tol). Would it be okay

  • to introduce keyword arguments abstol, reltol (mimicking the names of arguments in the SciML ecosystem) with the same meaning as in PETSc, i.e. to stop the iteration when current_residual <= max(reltol * initial_residual, abstol)?
  • to deprecate tol in favor of reltol, e.g. using something along the lines of the following code snippet?
    function bicgstabl!(x, A, b, l = 2;
                        abstol = zero(real(eltype(b))),
                        reltol = sqrt(eps(real(eltype(b)))),
                        tol = nothing, # TODO: deprecated
                        max_mv_products::Int = size(A, 2),
                        log::Bool = false,
                        verbose::Bool = false,
                        Pl = Identity(),
                        kwargs...)
    
        # TODO: deprecated
        if tol !== nothing
            Base.depwarn("The keyword argument `tol` is deprecated, use `reltol` instead.", :bicgstabl!)
            reltol = tol
        end
    
        # the rest of the implementation
    end

If it's okay, I can invest some work into this, but would like to know first whether it would be acceptable (to not waste some effort).

A related issue could be to specify a divergence tolerance divtol, similar to PETSc.

Another issue concerns the maximal number of iterations or matrix-vector products. Some methods allow specifying a maxiter (e.g. GMRES) while other allow specifying a max_mv_products (e.g. BiCGStab(l)). It would be nice if all methods could use the same keyword argument for this to make it easier to switch solvers.

@haampie
Copy link
Member

haampie commented Dec 8, 2020

Hi @ranocha, I would say this is great to have. abstol and reltol are good choices, I believe there's been some people complaining atol and rtol are hard to decipher.

The depwarn is OK, especially if iterativesolvers.jl is used somewhere deep inside another algorithm. But I'm also happy to quickly create a breaking change that doesn't have the depwarn.

max_mv_products is a mistake I'd say. Let's just use maxiter throughout.

divtol is quite alright too, but do note that there's various methods that have a bumpy convergence behavior, so it'll only really make sense for methods that are known to be not-bumpy, including CG and maybe GMRES. But even then, irregular convergence can happen due to rounding errors etc, so I probably wouldn't use this divtol myself.

@ranocha
Copy link
Member Author

ranocha commented Dec 8, 2020

Thanks for the feedback! I will add abstol and reltol in #280. We can have a look at maxiter(s) later, since that will likely be more difficult to change without breaking anything, since we should be consistent with the use of iterations in IterativeSolvers (in the convergence history etc.).

If we go for maxiter(s), I would like to use maxiters since that's in accordance with the keyword argument in the SciML ecosystem, see https://diffeq.sciml.ai/latest/basics/common_solver_opts/#Miscellaneous.

Would you propose to use maxiters as a replacement for "maximum number of matrix-vector products" and make this nomenclature consistent everywhere, i.e. should we also use iters to count the number of matrix-vector products in the convergence history or should iters still be the number of iterations?

@haampie
Copy link
Member

haampie commented Dec 8, 2020

Let's just make maxiter(s) the number of "outer" iterations of the algorithm. bicgstab(k) is special cause it has k gmres-like inner iteration per out iteration, but it's implemented as the matrix acting on a block of vectors, which is typically faster than applying it per vector, so it doesn't really make sense to count matrix-vector products in that algorithm. So instead, just count the number of outer iterations, and make the user responsible for potentially scaling that with their k param.

@haampie
Copy link
Member

haampie commented Dec 8, 2020

And yeah, maxiters is fine if that brings more consistency across packages.

@ranocha
Copy link
Member Author

ranocha commented Dec 8, 2020

Okay, that sounds reasonable to me.

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

No branches or pull requests

2 participants