check_manifold_point and check_tangent_vector throw right error types

[mateuszbaran]

I've corrected an error in check_manifold_point and check_tangent_vector. I'd suggest tagging a new minor release after merging this as it's technically breaking. I needed to change a few tests in JuliaManifolds/Manifolds.jl#46 due to this.

[codecov]

Codecov Report

Merging #15 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
+ Coverage   98.56%   98.63%   +0.07%     
==========================================
  Files           3        3              
  Lines         209      220      +11     
==========================================
+ Hits          206      217      +11     
  Misses          3        3

Impacted Files	Coverage Δ
src/ArrayManifold.jl	100% <100%> (ø)	⬆️
src/ManifoldsBase.jl	97.32% <100%> (+0.15%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9250e24...c1c04f8. Read the comment docs.

[kellertuer]

I like the names check_manifold_point – but how did the manifold_point_error name come up? Can we try to get a name that tells a little more what it does and why it is different from the check?

To be precise: The name of the function does not provide an action indicating, what the function does (like check or the previous is indicator).

Despite that, since it is breaking, I am ok with doing a version 0.1.1 with that.

[mateuszbaran]

So, what name would you suggest? Maybe get_manifold_point_error? It's a function that returns an error telling why an object doesn't represent a point on a manifold.

Despite that, since it is breaking, I am ok with doing a version 0.1.1 with that.

By new minor release I actually meant 0.2.0 (it's major.minor.patch).

[kellertuer]

Thanks for the clarification on the version number, then, of course 0.2.0

For the name. So as far as I see, both functions aim to do the same, despite the fact that one throws an error, the other just returns it. Can't we unify that into one function with an optional argument? I actually liked the names is_manifold_point or is_tangent_vector,
but I'd also be fine with the check names – and then maybe an optional (non keyword) parameter throwError which defaults to false?

Otherwise the functions are a little too similar to come up with two names, really – at least for me.

[mateuszbaran]

I really think that the functionality of manifold_point_error should remain in one place (though maybe under a different name), since it's the simplest possible way to provide error checking for manifolds. Defining it makes all checking work, whether it's a true/false test or error throwing. Methods of this function can be minimal, as they do not care how the error is handled further.

We can, however, unify is_manifold_point and check_manifold_point. We can leave is_manifold_point with the optional throw_error parameter that defaults to false. Does it look reasonable to you?

[kellertuer]

Ah, I missed something, we actually have three functions then (keeping old names for clarity)

• manifold_point_error – returns (but does not throw) and Error if something is wrong, otherwise nothing
• check_manfiold_point – throws the error from the first function, if it is not nothing, otherwise returns nothing
• is_manifold_point returns true if the first gives nothing, otherwise failse.

And I think it is reasonable to have a function that has no optional arguments nor throws an error, that just (attention!) checks a point for easy error checking and still getting information (like the first). And I am also for unifying the second and third. So what about

• check_manifold_point for the first, it checks (but does not "report" in form of throwing an error and provides more information than just true/false, so check seems resonable)
• is_manifold_point for the second and third, which can have a silent mode (not throwing errors) that we can call throw_error=false (or silent=true though that name might be less obvious).

What do you think?

[mateuszbaran]

So what about

• check_manifold_point for the first, it checks (but does not "report" in form of throwing an error and provides more information than just true/false, so check seems resonable)
• is_manifold_point for the second and third, which can have a silent mode (not throwing errors) that we can call throw_error=false (or silent=true though that name might be less obvious).

What do you think?

That's reasonable, I'll modify it this way.

[mateuszbaran]

I think I've modified the functions correctly.

[kellertuer]

Cool, that you also extended typed vector transports.