Repository: JuliaManifolds/ManifoldsBase.jl (julia)

Issue:
check_vector or is_vector – problem with iterated embeddings
In pur last PR we changed the line

https://github.com/JuliaManifolds/ManifoldsBase.jl/blob/5fa0a5c8b46694cdf8186c3668d40ae91f2ed809/src/decorator_trait.jl#L386

from calling ìs_vector` to `check_vector`. I just noticed a problem with that. Namely if we have iterated embeddings (General Grassmann is embedded into Generalised Stiefel is embedded into Euclidean) this only checks one level and does not iterate this (since the decorator is only done for `is_vector`. What do you think should we do about this? If possible I would like to switch back.

Pull request description:
This PR fixes #102 .

Patch:
diff --git a/Project.toml b/Project.toml
index a7f55262e..9a43dbd4e 100644
--- a/Project.toml
+++ b/Project.toml
@@ -1,7 +1,7 @@
 name = "ManifoldsBase"
 uuid = "3362f125-f0bb-47a3-aa74-596ffd7ef2fb"
 authors = ["Seth Axen <seth.axen@gmail.com>", "Mateusz Baran <mateuszbaran89@gmail.com>", "Ronny Bergmann <manopt@ronnybergmann.net>", "Antoine Levitt <antoine.levitt@gmail.com>"]
-version = "0.13.6"
+version = "0.13.7"
 
 [deps]
 LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"
diff --git a/src/decorator_trait.jl b/src/decorator_trait.jl
index 8527f1f06..83e11d489 100644
--- a/src/decorator_trait.jl
+++ b/src/decorator_trait.jl
@@ -147,7 +147,6 @@ function check_size(::TraitList{IsEmbeddedManifold}, M::AbstractDecoratorManifol
     end
     return nothing
 end
-
 # Introduce Deco Trait | automatic foward | fallback
 @trait_function check_size(M::AbstractDecoratorManifold, p, X)
 # Embedded
@@ -155,7 +154,7 @@ function check_size(::TraitList{IsEmbeddedManifold}, M::AbstractDecoratorManifol
     mpe = check_size(get_embedding(M, p), embed(M, p), embed(M, p, X))
     if mpe !== nothing
         return ManifoldDomainError(
-            "$X is not a tangent vector to $p on $M because it is not a valid tangent vector in its embedding.",
+            "$X is not a tangent vector at $p on $M because it is not a valid tangent vector in its embedding.",
             mpe,
         )
     end
@@ -335,14 +334,17 @@ function is_point(
         te && throw(es)
         return false
     end
-    mpe = check_point(get_embedding(M, p), embed(M, p); kwargs...)
-    if mpe !== nothing
-        wrapped_error = ManifoldDomainError(
-            "$p is not a point on $M because it is not a valid point in its embedding.",
-            mpe,
-        )
-        te && throw(wrapped_error)
-        return false
+    try
+        pt = is_point(get_embedding(M, p), embed(M, p), te; kwargs...)
+        !pt && return false # no error thrown (deactivated) but returned false -> return false
+    catch e
+        if e isa DomainError || e isa AbstractManifoldDomainError
+            e = ManifoldDomainError(
+                "$p is not a point on $M because it is not a valid point in its embedding.",
+                e,
+            )
+        end
+        throw(e) #an error occured that we do not handle ourselves -> rethrow.
     end
     mpe = check_point(M, p; kwargs...)
     mpe === nothing && return true
@@ -370,27 +372,39 @@ function is_vector(
     cbp = true;
     kwargs...,
 )
-    if cbp
-        # check whether p is valid before embedding the tangent vector
-        # throws it te=true
-        ep = is_point(M, p, te; kwargs...)
-        !ep && return false
-    end
-    # now that we know p is valid, check size of X
     es = check_size(M, p, X)
     if es !== nothing
         te && throw(es) # error & throw?
         return false
     end
+    if cbp
+        # check whether p is valid before embedding the tangent vector
+        # throws it te=true
+        try
+            ep = is_point(M, p, te; kwargs...)
+            !ep && return false
+        catch e
+            if e isa DomainError || e isa AbstractManifoldDomainError
+                e = ManifoldDomainError(
+                    "$X is not a tangent vector to $p on $M because its bas epoint is not valid point on $M.",
+                    e,
+                )
+            end
+            throw(e)
+        end
+    end
     # Check vector in embedding
-    mpe = check_vector(get_embedding(M, p), embed(M, p), embed(M, p, X); kwargs...)
-    if mpe !== nothing
-        wrapped_error = ManifoldDomainError(
-            "$X is not a tangent vector to $p on $M because it is not a valid tangent vector in its embedding.",
-            mpe,
-        )
-        te && throw(wrapped_error)
-        return false
+    try
+        tv = is_vector(get_embedding(M, p), embed(M, p), embed(M, p, X), te, cbp; kwargs...)
+        !tv && return false # no error thrown (deactivated) but returned false -> return false
+    catch e
+        if e isa DomainError || e isa AbstractManifoldDomainError
+            e = ManifoldDomainError(
+                "$X is not a tangent vector to $p on $M because it is not a valid tangent vector in its embedding.",
+                e,
+            )
+        end
+        throw(e)
     end
     # Check (additional) local stuff
     mtve = check_vector(M, p, X; kwargs...)
diff --git a/src/errors.jl b/src/errors.jl
index e99339756..cfd122a00 100644
--- a/src/errors.jl
+++ b/src/errors.jl
@@ -1,3 +1,10 @@
+"""
+    AbstractManifoldDomainError <: Exception
+
+An absytract Case for Errors when checking validity of points/vectors on mainfolds
+"""
+abstract type AbstractManifoldDomainError <: Exception end
+
 @doc """
     CompnentError{I,E} <: Exception
 
@@ -8,7 +15,7 @@ Store an error that occured in a component, where the additional `index` is stor
 * `index` index where the error occured`
 * `error` error that occured.
 """
-struct ComponentManifoldError{I,E} <: Exception where {I,E<:Exception}
+struct ComponentManifoldError{I,E} <: AbstractManifoldDomainError where {I,E<:Exception}
     index::I
     error::E
 end
@@ -25,7 +32,7 @@ with [`ComponentManifoldError`](@ref) to store a set of errors that occured.
 # Fields
 * `errors` a `Vector` of `<:Exceptions`.
 """
-struct CompositeManifoldError{T} <: Exception where {T<:Exception}
+struct CompositeManifoldError{T} <: AbstractManifoldDomainError where {T<:Exception}
     errors::Vector{T}
 end
 CompositeManifoldError() = CompositeManifoldError{Exception}(Exception[])
@@ -86,7 +93,7 @@ An error to represent a nested (Domain) error on a manifold, for example
 if a point or tangent vector is invalid because its representation in some
 embedding is already invalid.
 """
-struct ManifoldDomainError{E} <: Exception where {E<:Exception}
+struct ManifoldDomainError{E} <: AbstractManifoldDomainError where {E<:Exception}
     outer_text::String
     error::E
 end


Test patch:
diff --git a/test/embedded_manifold.jl b/test/embedded_manifold.jl
index c2bb9eda8..acb4ef9a1 100644
--- a/test/embedded_manifold.jl
+++ b/test/embedded_manifold.jl
@@ -206,19 +206,23 @@ ManifoldsBase.decorated_manifold(::FallbackManifold) = DefaultManifold(3)
         @test representation_size(M) == (3,)
         # Check point checks using embedding
         @test is_point(M, [1 0.1 0.1], true)
-        @test_throws DomainError is_point(M, [-1, 0, 0], true) #wrong dim (3,1)
+        @test !is_point(M, [-1, 0, 0]) #wrong dim (3,1)
+        @test_throws DomainError is_point(M, [-1, 0, 0], true)
         @test !is_point(M, [-1, 0, 0])
-        @test_throws ManifoldDomainError is_point(M, [1, 0.1], true) # size (from embedding)
+        @test !is_point(M, [1, 0.1]) # size (from embedding)
+        @test_throws ManifoldDomainError is_point(M, [1, 0.1], true)
         @test !is_point(M, [1, 0.1])
         @test is_point(M, [1 0 0], true)
         @test !is_point(M, [-1 0 0]) # right size but <0 1st
         @test_throws DomainError is_point(M, [-1 0 0], true) # right size but <0 1st
-        @test_throws ManifoldDomainError is_vector(M, [1 0 0], [1], true) # right point, wrong size vector
+        @test !is_vector(M, [1 0 0], [1]) # right point, wrong size vector
+        @test_throws ManifoldDomainError is_vector(M, [1 0 0], [1], true)
         @test !is_vector(M, [1 0 0], [1])
         @test_throws DomainError is_vector(M, [1 0 0], [-1 0 0], true) # right point, vec 1st <0
         @test !is_vector(M, [1 0 0], [-1 0 0])
         @test is_vector(M, [1 0 0], [1 0 1], true)
-        @test_throws DomainError is_vector(M, [-1, 0, 0], [0, 0, 0], true)
+        @test !is_vector(M, [-1, 0, 0], [0, 0, 0])
+        @test_throws ManifoldDomainError is_vector(M, [-1, 0, 0], [0, 0, 0], true)
         @test_throws DomainError is_vector(M, [1, 0, 0], [-1, 0, 0], true)
         @test !is_vector(M, [-1, 0, 0], [0, 0, 0])
         @test !is_vector(M, [1, 0, 0], [-1, 0, 0])