From b5b4bb18e255910910970b2c6fd280a456caa1a7 Mon Sep 17 00:00:00 2001 From: Ronny Bergmann Date: Fri, 18 Sep 2020 19:43:17 +0200 Subject: [PATCH] More rigorous size tests before allocation (#49) * introduces a small size test, such that allocations are only called with properly sized arrays in the embedding allocations. * runs formatter. * works on remarks from code review and adapts test to cover all errors. * increase code coverage. * Changes the default behaviour of `AbstractEmbeddedManifold` to not call `embed`, since representations are already embedded. * add tests for the new `check_size` (which might reduce code in `Manifolds` since these tests are often at the beginning of the point/tangent vector checks. * making EmbeddedManifold and AbstractEmbeddedManifold more coherent * Revert "making EmbeddedManifold and AbstractEmbeddedManifold more coherent" This reverts commit 8cc83f6f405ed97a2aa294a063248feb87cfc338. * Making `EmbeddedManifold` not an `AbstractEmbeddedManifold` * formatting * docfix * more fixes for embedded manifolds * mark embed and project in EmbeddedManifold as :parent * provides proper implementations for embedded manifolds following the new model we have, that it decorates M. * fix a bug in the order for project. * increase test coverage, fix a bug introduced in allocation but iadds a comment to explain this switch * reduce unnecessary code further. * increase test coverage slightly. Co-authored-by: Mateusz Baran --- Project.toml | 2 +- src/EmbeddedManifold.jl | 72 +++++++++++++++++++-------------------- src/ManifoldsBase.jl | 51 +++++++++++++++++++++++++++ test/embedded_manifold.jl | 32 +++++++++++------ 4 files changed, 110 insertions(+), 47 deletions(-) diff --git a/Project.toml b/Project.toml index 6fc691cc..42ec14d3 100644 --- a/Project.toml +++ b/Project.toml @@ -1,7 +1,7 @@ name = "ManifoldsBase" uuid = "3362f125-f0bb-47a3-aa74-596ffd7ef2fb" authors = ["Seth Axen ", "Mateusz Baran ", "Ronny Bergmann ", "Antoine Levitt "] -version = "0.9.2" +version = "0.9.3" [deps] LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" diff --git a/src/EmbeddedManifold.jl b/src/EmbeddedManifold.jl index dfccdf14..62cc2186 100644 --- a/src/EmbeddedManifold.jl +++ b/src/EmbeddedManifold.jl @@ -14,13 +14,14 @@ The embedding is further specified by an [`AbstractEmbeddingType`](@ref). This means, that technically an embedded manifold is a decorator for the embedding, i.e. functions of this type get, in the semi-transparent way of the -[`AbstractDecoratorManifold`](@ref), passed on to the embedding. +[`AbstractDecoratorManifold`](@ref), passed on to the embedding. This is in contrast with +[`EmbeddedManifold`](@ref) that decorates the embedded manifold. !!! note - - Points on an `AbstractEmbeddedManifold` are still represented using representation - of the embedded manifold. Use [`embed`](@ref) to go to the representation of the embedding - and [`project`](@ref) to go the other way. + + Points on an `AbstractEmbeddedManifold` are represented using representation + of the embedding. A [`check_manifold_point`](@ref) should first invoke + the test of the embedding and then test further constraints for the embedded manifold. """ abstract type AbstractEmbeddedManifold{𝔽,T<:AbstractEmbeddingType} <: AbstractDecoratorManifold{𝔽} end @@ -63,12 +64,14 @@ and logarithmic maps. struct TransparentIsometricEmbedding <: AbstractIsometricEmbeddingType end """ - EmbeddedManifold{𝔽, MT <: Manifold, NT <: Manifold, ET} <: AbstractEmbeddedManifold{𝔽, ET} + EmbeddedManifold{𝔽, MT <: Manifold, NT <: Manifold} <: AbstractDecoratorManifold{𝔽} A type to represent that a [`Manifold`](@ref) `M` of type `MT` is indeed an emebedded manifold and embedded into the manifold `N` of type `NT`. -Based on the [`AbstractEmbeddingType`](@ref) `ET`, this introduces methods for `M` by -passing them through to embedding `N`. +Points and tangent vectors are still represented in the representation of `M` and the +manifold `M` is being decorated (contrary to [`AbstractEmbeddedManifold`](@ref)). +To go to the representation in the embedding, the function [`embed`](@ref) should be used. +[`project`](@ref) can be used to go the other way. # Fields @@ -77,32 +80,24 @@ passing them through to embedding `N`. # Constructor - EmbeddedManifold(M, N, e=TransparentIsometricEmbedding()) + EmbeddedManifold(M, N) Generate the `EmbeddedManifold` of the [`Manifold`](@ref) `M` into the -[`Manifold`](@ref) `N` with [`AbstractEmbeddingType`](@ref) `e` that by default is the most -transparent [`TransparentIsometricEmbedding`](@ref) +[`Manifold`](@ref) `N`. """ -struct EmbeddedManifold{𝔽,MT<:Manifold{𝔽},NT<:Manifold,ET} <: AbstractEmbeddedManifold{𝔽,ET} +struct EmbeddedManifold{𝔽,MT<:Manifold{𝔽},NT<:Manifold} <: AbstractDecoratorManifold{𝔽} manifold::MT embedding::NT end -function EmbeddedManifold( - M::MT, - N::NT, - e::ET = TransparentIsometricEmbedding(), -) where {𝔽,MT<:Manifold{𝔽},NT<:Manifold,ET<:AbstractEmbeddingType} - return EmbeddedManifold{𝔽,MT,NT,ET}(M, N) -end function allocate_result(M::AbstractEmbeddedManifold, f::typeof(embed), x...) T = allocate_result_type(M, f, x) - return allocate(x[end], T, representation_size(decorated_manifold(M))) + return allocate(x[1], T, representation_size(decorated_manifold(M))) end function allocate_result(M::AbstractEmbeddedManifold, f::typeof(project), x...) T = allocate_result_type(M, f, x) - return allocate(x[end], T, representation_size(base_manifold(M))) + return allocate(x[1], T, representation_size(base_manifold(M))) end """ @@ -131,12 +126,11 @@ check whether a point `p` is a valid point on the [`AbstractEmbeddedManifold`](@ i.e. that `embed(M, p)` is a valid point on the embedded manifold. """ function check_manifold_point(M::AbstractEmbeddedManifold, p; kwargs...) - q = embed(M, p) return invoke( check_manifold_point, - Tuple{typeof(get_embedding(M)),typeof(q)}, + Tuple{typeof(get_embedding(M)),typeof(p)}, get_embedding(M), - q; + p; kwargs..., ) end @@ -158,20 +152,18 @@ function check_tangent_vector( mpe = check_manifold_point(M, p; kwargs...) mpe === nothing || return mpe end - q = embed(M, p) - Y = embed(M, p, X) return invoke( check_tangent_vector, - Tuple{typeof(get_embedding(M)),typeof(q),typeof(Y)}, + Tuple{typeof(get_embedding(M)),typeof(p),typeof(X)}, get_embedding(M), - q, - Y; + p, + X; check_base_point = check_base_point, kwargs..., ) end -decorated_manifold(M::AbstractEmbeddedManifold) = M.embedding +decorated_manifold(M::EmbeddedManifold) = M.embedding """ get_embedding(M::AbstractEmbeddedManifold) @@ -184,15 +176,23 @@ get_embedding(::AbstractEmbeddedManifold) return decorated_manifold(M) end -function show( - io::IO, - M::EmbeddedManifold{𝔽,MT,NT,ET}, -) where {𝔽,MT<:Manifold{𝔽},NT<:Manifold,ET<:AbstractEmbeddingType} - return print(io, "EmbeddedManifold($(M.manifold), $(M.embedding), $(ET()))") +""" + get_embedding(M::EmbeddedManifold) + +Return the [`Manifold`](@ref) `N` an [`EmbeddedManifold`](@ref) is embedded into. +""" +get_embedding(::EmbeddedManifold) + +function get_embedding(M::EmbeddedManifold) + return M.embedding +end + +function show(io::IO, M::EmbeddedManifold{𝔽,MT,NT}) where {𝔽,MT<:Manifold{𝔽},NT<:Manifold} + return print(io, "EmbeddedManifold($(M.manifold), $(M.embedding))") end function default_decorator_dispatch(M::EmbeddedManifold) - return default_embedding_dispatch(M) + return Val(true) end @doc doc""" diff --git a/src/ManifoldsBase.jl b/src/ManifoldsBase.jl index 8412bceb..a9a7e199 100644 --- a/src/ManifoldsBase.jl +++ b/src/ManifoldsBase.jl @@ -238,6 +238,52 @@ type. """ check_tangent_vector(M::Manifold, p, X; kwargs...) = nothing +""" + check_size(M::Manifold, p) + check_size(M::Manifold, p, X) + +Check whether `p` has the right [`representation_size`](@ref) for a [`Manifold`](@ref) `M`. +Additionally if a tangent vector is given, both `p` and `X` are checked to be of +corresponding correct representation sizes for points and tangent vectors on `M`. + +By default, `check_size` returns `nothing`, i.e. if no checks are implemented, the +assumption is to be optimistic. +""" +function check_size(M::Manifold, p) + n = size(p) + m = representation_size(M) + if length(n) != length(m) + return DomainError( + length(n), + "The point $(p) can not belong to the manifold $(M), since its size $(n) is not equal to the manifolds representation size ($(m)).", + ) + end + if n != m + return DomainError( + n, + "The point $(p) can not belong to the manifold $(M), since its size $(n) is not equal to the manifolds representation size ($(m)).", + ) + end +end +function check_size(M::Manifold, p, X) + mse = check_size(M, p) + mse === nothing || return mse + n = size(X) + m = representation_size(M) + if length(n) != length(m) + return DomainError( + length(n), + "The tangent vector $(X) can not belong to the manifold $(M), since its size $(n) is not equal to the manifolds representation size ($(m)).", + ) + end + if n != m + return DomainError( + n, + "The tangent vector $(X) can not belong to the manifold $(M), since its size $(n) is not equal to the manifodls representation size ($(m)).", + ) + end +end + """ distance(M::Manifold, p, q) @@ -294,6 +340,8 @@ the tangent spaces of the embedded base points. See also: [`EmbeddedManifold`](@ref), [`project`](@ref project(M::Manifold, p, X)) """ function embed(M::Manifold, p, X) + # Note that the order is switched, + # since the allocation by default takes the type of the first. Y = allocate_result(M, embed, X, p) embed!(M, Y, p, X) return Y @@ -651,6 +699,8 @@ Lie algebra is perfomed, too. See also: [`EmbeddedManifold`](@ref), [`embed`](@ref embed(M::Manifold, p, X)) """ function project(M::Manifold, p, X) + # Note that the order is switched, + # since the allocation by default takes the type of the first. Y = allocate_result(M, project, X, p) project!(M, Y, p, X) return Y @@ -820,6 +870,7 @@ export allocate, base_manifold, check_manifold_point, check_tangent_vector, + check_size, distance, exp, exp!, diff --git a/test/embedded_manifold.jl b/test/embedded_manifold.jl index 6149e80c..a435a185 100644 --- a/test/embedded_manifold.jl +++ b/test/embedded_manifold.jl @@ -51,25 +51,35 @@ struct NotImplementedEmbeddedManifold3 <: AbstractEmbeddedManifold{ℝ,DefaultEm ManifoldsBase.DefaultManifold(3), ) @test repr(M) == - "EmbeddedManifold($(sprint(show, M.manifold)), $(sprint(show, M.embedding)), TransparentIsometricEmbedding())" + "EmbeddedManifold($(sprint(show, M.manifold)), $(sprint(show, M.embedding)))" @test base_manifold(M) == ManifoldsBase.DefaultManifold(2) + @test get_embedding(M) == ManifoldsBase.DefaultManifold(3) @test ManifoldsBase.decorated_manifold(M) == ManifoldsBase.DefaultManifold(3) - @test ManifoldsBase.default_embedding_dispatch(M) === Val{false}() - @test ManifoldsBase.default_decorator_dispatch(M) === - ManifoldsBase.default_embedding_dispatch(M) + @test ManifoldsBase.default_decorator_dispatch(M) === Val(true) end + @testset "PlaneManifold" begin M = PlaneManifold() @test repr(M) == "PlaneManifold()" - @test ManifoldsBase.default_decorator_dispatch(M) === Val{false}() + @test ManifoldsBase.default_decorator_dispatch(M) === Val(false) + @test ManifoldsBase.default_embedding_dispatch(M) === Val(false) @test get_embedding(M) == ManifoldsBase.DefaultManifold(1, 3) # Check fallbacks to check embed->check_manifoldpoint Defaults - @test is_manifold_point(M, [1, 0], true) - @test is_tangent_vector(M, [1, 0], [1, 0, 0], true) - @test is_tangent_vector(M, [1, 0, 0], [0, 0], true) + @test_throws DomainError is_manifold_point(M, [1, 0, 0], true) + @test_throws DomainError is_manifold_point(M, [1 0], true) + @test is_manifold_point(M, [1 0 0], true) + @test_throws DomainError is_tangent_vector(M, [1 0 0], [1], true) + @test_throws DomainError is_tangent_vector(M, [1 0 0], [0 0 0 0], true) + @test is_tangent_vector(M, [1 0 0], [1 0 1], true) p = [1.0 1.0 0.0] q = [1.0 0.0 0.0] X = q - p + @test check_size(M, p) === nothing + @test check_size(M, p, X) === nothing + @test check_size(M, [1, 2]) isa DomainError + @test check_size(M, [1 2 3 4]) isa DomainError + @test check_size(M, p, [1, 2]) isa DomainError + @test check_size(M, p, [1 2 3 4]) isa DomainError @test embed(M, p) == p pE = similar(p) embed!(M, pE, p) @@ -105,8 +115,9 @@ struct NotImplementedEmbeddedManifold3 <: AbstractEmbeddedManifold{ℝ,DefaultEm @testset "Default Isometric Embedding Fallback Error Tests" begin M = NotImplementedEmbeddedManifold() A = zeros(2) - @test_throws ErrorException check_manifold_point(M, [1, 2]) - @test_throws ErrorException check_tangent_vector(M, [1, 2], [3, 4]) + # without any extra tests just the embedding is asked + @test check_manifold_point(M, [1, 2]) === nothing + @test check_tangent_vector(M, [1, 2], [3, 4]) === nothing @test norm(M, [1, 2], [2, 3]) ≈ sqrt(13) @test inner(M, [1, 2], [2, 3], [2, 3]) ≈ 13 @test_throws ErrorException manifold_dimension(M) @@ -169,6 +180,7 @@ struct NotImplementedEmbeddedManifold3 <: AbstractEmbeddedManifold{ℝ,DefaultEm @test_throws ErrorException embed(M3, [1, 2]) end end + @testset "EmbeddedManifold decorator dispatch" begin TM = NotImplementedEmbeddedManifold() # transparently iso IM = NotImplementedEmbeddedManifold2() # iso