Closed kellertuer closed 1 year ago
Merging #134 (0d3d23b) into master (555314c) will increase coverage by
0.00%
. The diff coverage is100.00%
.:exclamation: Current head 0d3d23b differs from pull request most recent head 48e2997. Consider uploading reports for the commit 48e2997 to get more accurate results
@@ Coverage Diff @@
## master #134 +/- ##
=======================================
Coverage 99.73% 99.74%
=======================================
Files 18 18
Lines 2297 2386 +89
=======================================
+ Hits 2291 2380 +89
Misses 6 6
Impacted Files | Coverage Δ | |
---|---|---|
src/decorator_trait.jl | 100.00% <ø> (ø) |
|
src/DefaultManifold.jl | 100.00% <100.00%> (ø) |
|
src/ManifoldsBase.jl | 98.58% <100.00%> (-0.74%) |
:arrow_down: |
src/ValidationManifold.jl | 100.00% <100.00%> (ø) |
|
src/errors.jl | 100.00% <100.00%> (ø) |
|
src/point_vector_fallbacks.jl | 99.25% <100.00%> (+0.01%) |
:arrow_up: |
src/numbers.jl | 100.00% <0.00%> (ø) |
|
src/PowerManifold.jl | 99.79% <0.00%> (+0.40%) |
:arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
No problem, I've fixed the ambiguities :slightly_smiling_face: .
ah, stupid me, sure the bool has to be more unambiguous then as well :) Thanks for the help.
I will think next about the string to display for the Isapprox and after that check for the tests; I did some info/warn checks in Manopt but for now I was not able to adapt them.
I got a little stuck thinking about the isapprox
function here, because different from is_point
where the internal check_point
delivers a message, the lower level (undecorated or implemented at concrete manifold level) isapprox
es not only do their own isapprox implementation (which we can not recreate on the abstract level afterwards to check what the reason was), they also do not return any reasoning. So for the abstract case it might be hard to provide @info/@warn things, for the concrete cases it would be a lot of updates in Manifolds.jl
We could change to an internal is_approx
(similar to the internal check_point
( which would mean not so much change in Manifolds but still a change (to introduce reasons).
Also I still did not yet get tests for @info/@warn to work today either.
We could change to an internal
is_approx
(similar to the internalcheck_point
( which would mean not so much change in Manifolds but still a change (to introduce reasons).
I think that's a reasonable direction. Maybe check_approx
would be a good name for that new function?
check_approx
is a very good name idea, this way we could introduce that as nonbreaking here (have an idea already) and update the functions over on Manifolds step by step.
I sketched check_approx
including a fallback to the (old/previous) isapprox
so that the default is backwards-compatible (but we can update them to check_approx
es to provide more information. I am just wondering: Should we do a new Error Type for that? I feel AssertionError
is too vague and would maybe define ApproxError
or something like that – maybe even two – one for points one for tangents. Any ideas for good names?
Sure, we can have a special error type for that. ApproxError
would be fine.
Uh, the is approx has a large ambiguity problem. Compare
isapprox(M, p, q, error::Symbol)
and
isapprox(M::PowerManifold, p, X, Y)
I do not yet know how to resolve that. Probably we can only do the error as a keyword? This would be inconsistent to the other ones and changing all to keywords would be a breaking change.
Yes, that's an issue. There is no good way to resolve such ambiguity and error
needs to be a keyword argument. It's inconsistent but we can live with that, and add to the list of changes for the next breaking release.
Ok, that sounds like an idea.
Now my only problem left is that currently the (old) default fallback results in a stack overflow since it is “running in circles”. I have to check how to best resolve that since we do not have a new positional argument to dispatch on for the new – more informative – case.
edit: I just decided to stop being stupid and just passed the default check_approx
to the nonmanifold-approx and that resolved the issue. This way all defaults work again, all isapprox
e in Manifolds.jl still work but we of course will transition to the check then.
Now the last thing missing is proper tests for all the new things.
I think I will also try to remove the 4 warnings the tests currently print by using @test_logs (:warn,) ...
if I can get that to work.
Besides Tests this should be ready for review.
Note that I changed one thing in the (old) tests: Those tests that threw warnings are now checked to issue the warning we expect.
I.e. that the following methods have symbol positional arguments
is_point
is_vector
isapprox
(with manifolds)This resolves #131.
I am not yet sure how to do the tests (I am not able to use
@test_logs
here and the actual info/warn string forisapprox
seems to be a little difficult still.edit: Oh and I introduced quite a few ambiguities, all but 3 I could resolve, those are special ones with the embedding and might be a little tricky.