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 8cc83f6. * 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 <[email protected]>
JuliaManifolds · Sep 18, 2020 · b5b4bb1 · b5b4bb1 · mateuszbaran · Sep 18, 2020
1 parent 8789cab
commit b5b4bb1
Show file tree

Hide file tree

Showing 4 changed files with 110 additions and 47 deletions.
diff --git a/Project.toml b/Project.toml
@@ -1,7 +1,7 @@
 name = "ManifoldsBase"
 uuid = "3362f125-f0bb-47a3-aa74-596ffd7ef2fb"
 authors = ["Seth Axen <[email protected]>", "Mateusz Baran <[email protected]>", "Ronny Bergmann <[email protected]>", "Antoine Levitt <[email protected]>"]
-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
@@ -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
@@ -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
@@ -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