Introduce Aqua.jl-based checks (#379)

* Starts including an Aqua test.
* Reduce ambiguities
* Update changelog.
* add a badge.
* fix typos
* Fix test coverage.
* Unify/Simplify last stepsize.
* Update a comment in the current ambiguities.

---------

Co-authored-by: Mateusz Baran <[email protected]>

Changelog.md

@@ -5,6 +5,17 @@ All notable Changes to the Julia package `Manopt.jl` will be documented in this
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [0.4.61] unreleased
+
+### Added
+
+* Tests now also use `Aqua.jl` to spot problems in the code, e.g. ambiguities.
+
+### Fixed
+
+* `get_last_stepsize` was defined in quite different ways that caused ambiguities. That is now internally a bit restructured and should work nicer.
+  Internally this means that the interims dispatch on `get_last_stepsize(problem, state, step, vars...)` was removed. Now the only two left are `get_last_stepsize(p, s, vars...)` and the one directly checking `get_last_stepsize(::Stepsize)` for stored values.
+
 ## [0.4.60] – April 10, 2024
 
 ### Added

Project.toml

@@ -13,14 +13,12 @@ LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"
 ManifoldDiff = "af67fdf4-a580-4b9f-bbec-742ef357defd"
 ManifoldsBase = "3362f125-f0bb-47a3-aa74-596ffd7ef2fb"
 Markdown = "d6f4376e-aef5-505a-96c1-9c027394607a"
-PolynomialRoots = "3a141323-8675-5d76-9d11-e1df1406c778"
 Preferences = "21216c6a-2e73-6563-6e65-726566657250"
 Printf = "de0858da-6303-5e67-8744-51eddeeeb8d7"
 Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"
 Requires = "ae029012-a4dd-5104-9daa-d747884805df"
 SparseArrays = "2f01184e-e22b-5df5-ae63-d93ebab69eaf"
 Statistics = "10745b16-79ce-11e8-11f9-7d13ad32a3b2"
-Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
 
 [weakdeps]
 JuMP = "4076af6c-e467-56ae-b986-b466b2749572"
@@ -40,20 +38,23 @@ ManoptPlotsExt = "Plots"
 ManoptRipQPQuadraticModelsExt = ["RipQP", "QuadraticModels"]
 
 [compat]
+Aqua = "0.8"
 ColorSchemes = "3.5.0"
 ColorTypes = "0.9.1, 0.10, 0.11"
 Colors = "0.11.2, 0.12"
 DataStructures = "0.17, 0.18"
 Dates = "1.6"
+ForwardDiff = "0.10"
 JuMP = "1.15"
 LRUCache = "1.4"
 LinearAlgebra = "1.6"
+LineSearches = "7.2.0"
 ManifoldDiff = "0.3.8"
 Manifolds = "0.9.11"
 ManifoldsBase = "0.15.8"
 ManoptExamples = "0.1.4"
 Markdown = "1.6"
-PolynomialRoots = "1"
+Plots = "1.30"
 Preferences = "1.4"
 Printf = "1.6"
 QuadraticModels = "0.9"
@@ -66,6 +67,7 @@ Test = "1.6"
 julia = "1.6"
 
 [extras]
+Aqua = "4c88cf16-eb10-579e-8560-4a9242c79595"
 ForwardDiff = "f6369f11-7733-5829-9624-2563aa707210"
 JuMP = "4076af6c-e467-56ae-b986-b466b2749572"
 LRUCache = "8ac3fa9e-de4c-5943-b1dc-09c6b5f20637"
@@ -79,4 +81,4 @@ RipQP = "1e40b3f8-35eb-4cd8-8edd-3e515bb9de08"
 Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
 
 [targets]
-test = ["Test", "ForwardDiff", "JuMP", "Manifolds", "ManoptExamples", "ManifoldDiff", "Plots", "LineSearches", "LRUCache", "RipQP", "QuadraticModels"]
+test = ["Test", "Aqua", "ForwardDiff", "JuMP", "Manifolds", "ManoptExamples", "ManifoldDiff", "Plots", "LineSearches", "LRUCache", "RipQP", "QuadraticModels"]

Readme.md

@@ -6,6 +6,8 @@ Optimization Algorithm on Riemannian Manifolds.
 [![Code Style: Blue](https://img.shields.io/badge/code%20style-blue-4495d1.svg)](https://github.com/invenia/BlueStyle)
 [![CI](https://github.com/JuliaManifolds/Manopt.jl/workflows/CI/badge.svg)](https://github.com/JuliaManifolds/Manopt.jl/actions?query=workflow%3ACI+branch%3Amaster)
 [![codecov](https://codecov.io/gh/JuliaManifolds/Manopt.jl/branch/master/graph/badge.svg)](https://codecov.io/gh/JuliaManifolds/Manopt.jl)
+[![Aqua QA](https://raw.githubusercontent.com/JuliaTesting/Aqua.jl/master/badge.svg)](https://github.com/JuliaTesting/Aqua.jl)
+
 [![DOI](https://zenodo.org/badge/74746729.svg)](https://zenodo.org/badge/latestdoi/74746729)
 [![DOI](https://joss.theoj.org/papers/10.21105/joss.03866/status.svg)](https://doi.org/10.21105/joss.03866)
 

ext/ManoptManifoldsExt/ManoptManifoldsExt.jl

@@ -8,7 +8,9 @@ import Manopt:
     alternating_gradient_descent!,
     get_gradient,
     get_gradient!,
-    set_manopt_parameter!
+    set_manopt_parameter!,
+    reflect,
+    reflect!
 using LinearAlgebra: cholesky, det, diag, dot, Hermitian, qr, Symmetric, triu, I, Diagonal
 import ManifoldsBase: copy, mid_point, mid_point!
 

ext/ManoptManifoldsExt/manifold_functions.jl

@@ -58,3 +58,73 @@ function mid_point!(M::Sphere, y, p, q, x)
     y .= exp(M, p, 0.5 * X)
     return y
 end
+
+@doc raw"""
+    reflect(M, f, x; kwargs...)
+    reflect!(M, q, f, x; kwargs...)
+
+reflect the point `x` from the manifold `M` at the point `f(x)` of the
+function ``f: \mathcal M → \mathcal M``, given by
+
+````math
+    \operatorname{refl}_f(x) = \operatorname{refl}_{f(x)}(x),
+````
+
+Compute the result in `q`.
+
+see also [`reflect`](@ref reflect(M::AbstractManifold, p, x))`(M,p,x)`, to which the keywords are also passed to.
+"""
+reflect(M::AbstractManifold, pr::Function, x; kwargs...) = reflect(M, pr(x), x; kwargs...)
+function reflect!(M::AbstractManifold, q, pr::Function, x; kwargs...)
+    return reflect!(M, q, pr(x), x; kwargs...)
+end
+
+@doc raw"""
+    reflect(M, p, x, kwargs...)
+    reflect!(M, q, p, x, kwargs...)
+
+Reflect the point `x` from the manifold `M` at point `p`, given by
+
+````math
+    \operatorname{refl}_p(x) = \operatorname{retr}_p(-\operatorname{retr}^{-1}_p x).
+````
+
+where ``\operatorname{retr}`` and ``\operatorname{retr}^{-1}`` denote a retraction and an inverse
+retraction, respectively.
+This can also be done in place of `q`.
+
+## Keyword arguments
+
+* `retraction_method`:         (`default_retraction_metiod(M, typeof(p))`) the retraction to use in the reflection
+* `inverse_retraction_method`: (`default_inverse_retraction_method(M, typeof(p))`) the inverse retraction to use within the reflection
+
+and for the `reflect!` additionally
+
+* `X`:                         (`zero_vector(M,p)`) a temporary memory to compute the inverse retraction in place.
+  otherwise this is the memory that would be allocated anyways.
+"""
+function reflect(
+    M::AbstractManifold,
+    p,
+    x;
+    retraction_method=default_retraction_method(M, typeof(p)),
+    inverse_retraction_method=default_inverse_retraction_method(M, typeof(p)),
+    X=nothing,
+)
+    return retract(
+        M, p, -inverse_retract(M, p, x, inverse_retraction_method), retraction_method
+    )
+end
+function reflect!(
+    M::AbstractManifold,
+    q,
+    p,
+    x;
+    retraction_method=default_retraction_method(M),
+    inverse_retraction_method=default_inverse_retraction_method(M),
+    X=zero_vector(M, p),
+)
+    inverse_retract!(M, X, p, x, inverse_retraction_method)
+    X .*= -1
+    return retract!(M, q, p, X, retraction_method)
+end

src/plans/Douglas_Rachford_plan.jl

@@ -1,3 +1,4 @@
+function reflect end
 @doc raw"""
     reflect(M, f, x; kwargs...)
     reflect!(M, q, f, x; kwargs...)
@@ -13,10 +14,7 @@ Compute the result in `q`.
 
 see also [`reflect`](@ref reflect(M::AbstractManifold, p, x))`(M,p,x)`, to which the keywords are also passed to.
 """
-reflect(M::AbstractManifold, pr::Function, x; kwargs...) = reflect(M, pr(x), x; kwargs...)
-function reflect!(M::AbstractManifold, q, pr::Function, x; kwargs...)
-    return reflect!(M, q, pr(x), x; kwargs...)
-end
+reflect(M::AbstractManifold, pr::Function, x; kwargs...)
 
 @doc raw"""
     reflect(M, p, x, kwargs...)
@@ -42,28 +40,4 @@ and for the `reflect!` additionally
 * `X`:                         (`zero_vector(M,p)`) a temporary memory to compute the inverse retraction in place.
   otherwise this is the memory that would be allocated anyways.
 """
-function reflect(
-    M::AbstractManifold,
-    p,
-    x;
-    retraction_method=default_retraction_method(M, typeof(p)),
-    inverse_retraction_method=default_inverse_retraction_method(M, typeof(p)),
-    X=nothing,
-)
-    return retract(
-        M, p, -inverse_retract(M, p, x, inverse_retraction_method), retraction_method
-    )
-end
-function reflect!(
-    M::AbstractManifold,
-    q,
-    p,
-    x;
-    retraction_method=default_retraction_method(M),
-    inverse_retraction_method=default_inverse_retraction_method(M),
-    X=zero_vector(M, p),
-)
-    inverse_retract!(M, X, p, x, inverse_retraction_method)
-    X .*= -1
-    return retract!(M, q, p, X, retraction_method)
-end
+reflect(M::AbstractManifold, p::Any, x; kwargs...)

src/plans/hessian_plan.jl

@@ -406,7 +406,11 @@ function update_hessian!(
 end
 
 function update_hessian!(
-    M, f::ApproxHessianSymmetricRankOne{InplaceEvaluation}, p, p_proposal, X
+    M::AbstractManifold,
+    f::ApproxHessianSymmetricRankOne{InplaceEvaluation},
+    p,
+    p_proposal,
+    X,
 )
     grad_proposal = zero_vector(M, p_proposal)
     f.gradient!!(M, grad_proposal, p_proposal)
@@ -519,7 +523,9 @@ function (f::ApproxHessianBFGS{InplaceEvaluation})(M, Y, p, X)
     return Y
 end
 
-function update_hessian!(M, f::ApproxHessianBFGS{AllocatingEvaluation}, p, p_proposal, X)
+function update_hessian!(
+    M::AbstractManifold, f::ApproxHessianBFGS{AllocatingEvaluation}, p, p_proposal, X
+)
     yk_c = get_coordinates(
         M,
         p,

src/plans/plan.jl

@@ -56,12 +56,21 @@ the optimisation on manifolds is different from the usual “experience” in
 (classical, Euclidean) optimization.
 Any other value has the same effect as not setting it.
 """
-function get_manopt_parameter(
-    e::Symbol, args...; default=get_manopt_parameter(Val(e), Val(:default))
+function get_manopt_parameter( # ignore args.
+    e::Symbol,
+    args...;
+    default=get_manopt_parameter(Val(e), Val(:default)),
 )
     return @load_preference("$(e)", default)
 end
-# Handle empty defaults
+function get_manopt_parameter( # reduce ambiguity, ignore s and args
+    e::Symbol,
+    s::Symbol,
+    args...;
+    default=get_manopt_parameter(Val(e), Val(:default)),
+)
+    return @load_preference("$(e)", default)
+end# Handle empty defaults
 get_manopt_parameter(::Symbol, ::Val{:default}) = nothing
 get_manopt_parameter(::Val{:Mode}, v::Val{:default}) = nothing
 

src/plans/quasi_newton_plan.jl

@@ -642,8 +642,11 @@ end
 function QuasiNewtonCautiousDirectionUpdate(
     update::U; θ::Function=x -> x
 ) where {
-    U<:Union{QuasiNewtonMatrixDirectionUpdate,QuasiNewtonLimitedMemoryDirectionUpdate{T}}
-} where {T<:AbstractQuasiNewtonUpdateRule}
+    U<:Union{
+        QuasiNewtonMatrixDirectionUpdate,
+        QuasiNewtonLimitedMemoryDirectionUpdate{T} where T<:AbstractQuasiNewtonUpdateRule,
+    },
+}
     return QuasiNewtonCautiousDirectionUpdate{U}(update, θ)
 end
 (d::QuasiNewtonCautiousDirectionUpdate)(mp, st) = d.update(mp, st)

src/plans/record.jl

@@ -649,10 +649,6 @@ mutable struct RecordEntryChange{TStorage<:StoreStateAction} <: RecordAction
     function RecordEntryChange(f::Symbol, d, a::StoreStateAction=StoreStateAction([f]))
         return new{typeof(a)}(Float64[], f, d, a)
     end
-    function RecordEntryChange(v, f::Symbol, d, a::StoreStateAction=StoreStateAction([f]))
-        update_storage!(a, Dict(f => v))
-        return new{typeof(a)}(Float64[], f, d, a)
-    end
 end
 function (r::RecordEntryChange)(
     amp::AbstractManoptProblem, ams::AbstractManoptSolverState, i::Int
@@ -820,7 +816,6 @@ function RecordFactory(s::AbstractManoptSolverState, a::Array{<:Any,1})
     i = findlast(x -> (isa(x, Pair)) && (x.first == :Iteration), b)
     if !isnothing(i)
         iter = popat!(b, i) #
-        println(iter, "<-")
         b = [b..., :Iteration => [iter.second..., iter_entries...]]
     else
         (length(iter_entries) > 0) && (b = [b..., :Iteration => iter_entries])

src/plans/solver_state.jl

@@ -415,27 +415,22 @@ end
 @noinline function StoreStateAction(
     M::AbstractManifold;
     store_fields::Vector{Symbol}=Symbol[],
-    store_points::Union{Type{TPS},Vector{Symbol}}=Tuple{},
-    store_vectors::Union{Type{TTS},Vector{Symbol}}=Tuple{},
+    store_points::Union{Type{<:Tuple},Vector{Symbol}}=Tuple{},
+    store_vectors::Union{Type{<:Tuple},Vector{Symbol}}=Tuple{},
     p_init=rand(M),
     X_init=zero_vector(M, p_init),
     once=true,
-) where {TPS<:Tuple,TTS<:Tuple}
-    if store_points isa Vector{Symbol}
-        TPS_tuple = tuple(store_points...)
-    else
-        TPS_tuple = Tuple(TPS.parameters)
-    end
-    if store_vectors isa Vector{Symbol}
-        TTS_tuple = tuple(store_vectors...)
-    else
-        TTS_tuple = Tuple(TTS.parameters)
-    end
+)
+    TPS_tuple = _store_to_tuple(store_points)
+    TTS_tuple = _store_to_tuple(store_vectors)
     point_values = NamedTuple{TPS_tuple}(map(_ -> p_init, TPS_tuple))
     vector_values = NamedTuple{TTS_tuple}(map(_ -> X_init, TTS_tuple))
     return StoreStateAction(store_fields, point_values, vector_values, once; M=M)
 end
 
+_store_to_tuple(store::Type{<:Tuple}) = Tuple(store.parameters)
+_store_to_tuple(store::Vector{Symbol}) = tuple(store...)
+
 @generated function extract_type_from_namedtuple(::Type{nt}, ::Val{key}) where {nt,key}
     for i in 1:length(nt.parameters[1])
         if nt.parameters[1][i] === key