JuliaManifolds / ManifoldsBase.jl

Basic interface for manifolds in Julia
https://juliamanifolds.github.io/ManifoldsBase.jl/
MIT License
87 stars 8 forks source link

Improve error message for embedded manifold is_point/is_vector #101

Closed kellertuer closed 2 years ago

kellertuer commented 2 years ago

(Yet another small update) to have nicer error messages for the case of embedded manifolds when the check for validity in the embedding (already) fails.

I am not sure whether we might have missed a check_size in the embedding here (that is one thing that is not done in the newly used check_ compare to the old is_), and we still have to check for test coverage.

codecov[bot] commented 2 years ago

Codecov Report

Merging #101 (7ee4e66) into master (d72f79c) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #101   +/-   ##
=======================================
  Coverage   99.80%   99.80%           
=======================================
  Files          17       17           
  Lines        2052     2068   +16     
=======================================
+ Hits         2048     2064   +16     
  Misses          4        4           
Impacted Files Coverage Δ
src/ManifoldsBase.jl 99.22% <ø> (ø)
src/decorator_trait.jl 100.00% <100.00%> (ø)
src/errors.jl 100.00% <100.00%> (ø)

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

kellertuer commented 2 years ago

Besides test coverage, I am surprised my code fails on Julia 1.0, I still have to check why, because mpe can not be nothing when entering the ManifoldDomainError (the if around) but it somehow still does end up being so.

mateuszbaran commented 2 years ago

Where do you see it failing on Julia 1.0? Tests pass now.

kellertuer commented 2 years ago

I see what happened, I had a typo in the previous commit which I even pushed (separately) so it triggered the tests and that failed.

So then only test coverage is left to check.

kellertuer commented 2 years ago

I hopefully just finished coverage (and added the same style to check size).

Currently the inner error is printed as

[0, 0, 0] is not a tangent vector to [1, 1, 0] on PosQuadrantManifold() because it is not a valid tangent vector in its embedding:
DomainError(0, "X[1] ≤ 0")

Can this maybe improved / is there a nicer print for errors in strings?

kellertuer commented 2 years ago

Can this maybe improved / is there a nicer print for errors in strings?

I stole an idea from our other error prints (changing the last print in the show error to another show error and the error now prints as

ManifoldDomainError: [0, 0, 0] is not a point on PosQuadrantManifold() because it is not a valid point in its embedding.
DomainError with 0:
p[1] ≤ 0