Kellertuer/fix check nested embeddings

[kellertuer]

This PR fixes #102 .

[codecov[bot]]

Codecov Report

Merging #104 (70a3148) into master (5fa0a5c) will decrease coverage by 0.04%. The diff coverage is 100.00%.

:exclamation: Current head 70a3148 differs from pull request most recent head 2b7de1c. Consider uploading reports for the commit 2b7de1c to get more accurate results

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
- Coverage   99.80%   99.75%   -0.05%     
==========================================
  Files          17       17              
  Lines        2068     2069       +1     
==========================================
  Hits         2064     2064              
- Misses          4        5       +1     

Impacted Files	Coverage Δ
src/errors.jl	100.00% <ø> (ø)
src/decorator_trait.jl	100.00% <100.00%> (ø)
src/ManifoldsBase.jl	97.67% <0.00%> (-1.56%)	:arrow_down:
src/ValidationManifold.jl	99.48% <0.00%> (-0.52%)	:arrow_down:
src/numbers.jl	100.00% <0.00%> (ø)
src/PowerManifold.jl	99.77% <0.00%> (+0.44%)	:arrow_up:

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

[kellertuer]

I think we can also omit the decoration of check_size again now since we are back to calling the (already decorated= is_ functions again?

[kellertuer]

One thing I would like to change is the default of embed if that is ok, in the old variant the default often was to just return p or X (for points, vectors, respectively) now several tests fail because with a wrong size (only checked after embed) the allocation/copyto fails. So I would prefer to have an embed that maybe does not fall back to embed!? What do you think?

[mateuszbaran]

I think we can also omit the decoration of check_size again now since we are back to calling the (already decorated= is_ functions again?

I'm not sure actually. Decorators can introduce additional valid sizes (i.e. affine matrices for SE(n)), so if someone wrapped SE(n) in something we would probably want to go through undecorating?

So I would prefer to have an embed that maybe does not fall back to embed!? What do you think?

I don't have a preference here. We have to extend the non-mutating embed for some manifolds either way.

[kellertuer]

Ok, seems reasonable to keep check_size with traits.

For embed, I currently added 2 times 2 lines with defaults for embed and I think it is about 4 or 5 further ones. So we can also keep the allocating default, I think. I am also not 100% sure what would be better.

[kellertuer]

I think we can stick with the allocation, currently in Manifolds there is only about 10 tests failing due to that and I will fix these next then.

So here, just code coverage is missing.

edit: so the main problem with the allocation is the following: Then doing check_size we now fist check the embedding, but when we embed(M,p) new points the allocation currently automatically allocates a variable the same size as the embedding. This (a) yields strange points if p is not of correct size and (b) errors if p is larger. To get closer to the behaviour of just the identity as an embedding, I will change this to allocate a new variable the same size (and type) as p, the size check will then (again) fail correctly.

edit2: Hm that's a little more complicated than I thought, because if the embedding is really larger and we do not just have the identity, then we really need the allocation from the embedding size.

[mateuszbaran]

Well, we have to either special-case allocation of embed (by adding methods to allocate_result) or write both mutating and non-mutating embed for embeddings that change the size.

[kellertuer]

Oh I think it is more a decision what to do as a default (for identity embeddings).

Up to the embed default we had (often) as a default embed(M,p) = p - does not check size, really just says: it is really the identity Now we usually have to allocate (right size of embedding) and copy over p, which I think is nicer, but fails if p is too large.

I just checked and the old default was assumed to work for about 3 or 4 manifolds, where I now manually introduced that. So we can leave this here as is I think. We could also check that p is of right size within embed, if you like, but I am now fine with the current state I think.

[kellertuer]

The remaining 6 lines we basically never hit a) the return false, because if throw_error(or te) is false, then the try block does not throw an error, so we never reach the return false. Same for the throwing of other errors, we currently do not have a check that goes wrong on purpose and divides by zero or something.

[mateuszbaran]

I think instead of

                te && throw(ew)
                return false
            end
            throw(e) #not an error we handle here

we could just have

            throw(e) #not an error we handle here

which in all reasonable cases will work the same and not leave anything not covered.

EDIT:

No, it actually won't work the same way.

EDIT 2:

                throw(ew)
            end
            throw(e) #not an error we handle here

should be fine though.

[kellertuer]

Oh now I chose nearly the first (just renaming (ew) to e again as is was in the very beginning.