Simplify the basis interface

[kellertuer]

This is a sketch for now and aims to simplify the basis type. What this aims to do is the following

We have several types that mark what type of a basis we want

• ArbitraryBasis
• ArbitraryOrthonormalBasis
• DiagonalizingOrthonormalBasis
  all of this are <: ArbitraryBasis

and for each we previously had a Precomputed basis type. This PR aims to provide just one CachedBasis <: ArbitraryBasis, that is internally typed by an arbitrary basis itself and has just one field for stored data. It usually stores just the vectors. For the last type it also stores the direction and the eigenvalues in a seperate type.

I am not yet done with all rework, but I hope this avoid the double-types (T and PrecomputedT) for all basis types. This is not an urgent PR, but I will work on this, when I have time and the current other PRs are finished.

[mateuszbaran]

Thanks, this approach should work I think. We could also rename get_basis to something like cache_basis to make it more clear what it does.

[kellertuer]

I have to rework Bundle and Product and such, that's why I waited for some feedback – thanks.

I think you get a basis anyways – so get_basis is ok – if you pass a cached one, it just passes that back, so that's fine. I would however implement get_coordinates and get_vector also once for cached basis, where they do more inners.
This should account for the case that the ad-hoc-basis generation that is now the default might be time-consuming (more time consuming than the memory consumption of caching).

[mateuszbaran]

I would however implement get_coordinates and get_vector also once for cached basis, where they do more inners.

As far as I can see get_coordinates is currently only implemented twice (one variant for real-bases and one for the other (it doesn't account for things like complex bases of quaternionic manifolds but it doesn't seem particularly useful).

This should account for the case that the ad-hoc-basis generation that is now the default might be time-consuming (more time consuming than the memory consumption of caching).

I don't see where the ad-hoc basis generation is the default. I was trying to make it so full basis generation doesn't happen unless the user specifically asks for it.

[kellertuer]

I don't see where the ad-hoc basis generation is the default. I was trying to make it so full basis generation doesn't happen unless the user specifically asks for it.

Oh that's what I meant with "ad-hoc", to avoid full basis generation. If you have a basis already stored, the user might just use that one. I will take care that that's the case and documented, then.

[kellertuer]

I finished a few tasks today, the next thing – and that might require some internal tricks – is representing cached bases on product manifolds – I will try to understand the old approach and mimick that within the new structure.

but for the base manifolds everything should already work.

[mateuszbaran]

This looks quite nice 👍 .

[kellertuer]

I am quite stuck on Product and Power to unify my cached approach. It seems I am missing something and I mainly do not understand how the old storage worked. I am missing length of my data every other time. Can you help me out here @mateuszbaran ? You know those two by heart.

My idea is to basically store a tuple of cached bases and return the old style representation of get_vectors – but I do not see how the previous precomputed storage worked nor how that was accessed. The internal storage might be very efficient but it's too clever for me to understand.

[mateuszbaran]

What do you mean by missing the length of your data?

ONBs for the power manifold, for example, work by storing a basis for for each particular component. For example if you have an SPD-valued image, we have a separate basis at each point stored. That's what PrecomputedPowerOrthonormalBasis stores. Then get_vector and get_coordinates iterate over all point in such image and use the stored basis to convert between tangent vectors and their coordinates. Product manifolds work similarly, just with tuples of bases instead of arrays of them.

My idea is to basically store a tuple of cached bases

Yes, that's what old style cached ONBs for product manifold do.

and return the old style representation of get_vectors

What do you mean by this?

[kellertuer]

Yes, the idea is clear, just the realisation/implementation – I can't get that to work. I tried to do the same approach as the original Precomputed... did (with a .parts) field but I am only running from one error message to another without understanding where they come from and especially not how to solve them. I seem to miss something in my understanding.

For example calling get_vectors seems sometimes to work currently? But the result then never passes any test. So it seems to return something different currently and I am not able to investigate this really. Actually I don't really know where to start :/. Maybe in the long run it would be nice to have a documentation of the internal functionality of both power and product manifolds. I tried to go line by line through the code but somehow, somewhere I seem to not understand what your code is doing, so I can't adapt it (to what I think is the same, also having a .parts field in its struct).

[mateuszbaran]

Unfortunately both power and product manifolds are quite complicated, yes. I was focusing on performance and versatility so the code is not that simple. Some things like zip_tuples is working around Julia's performance problems.

get_vectors is probably a poor place to start. I haven't worked out a specialized implementation for the power manifold for example (the generic one should work though).

I'm not really sure where the problem may be, the rework should just be about replacing the struct with cached basis with another? I'll take a closer look what's wrong here during the weekend. So push whatever you feel like and I'll see what's wrong with it.

[kellertuer]

Thanks for taking a look. Yes, I am not saying, you should've documented it directly; of course first it should work and be performant :) I am just stuck and don't know what's going wrong, since I mainly tried to follow your approaches, even storing in .parts. And I am mainly following the route: Replace all PrecomputedX by the CachedBasis with an internal data representation.

And well – get_vectors is the first thing that fails – for the decorator that was a good way to go through the functions – just test by test. But I'll try again tomorrow, maybe after all afternoon trying to figure out what the code is doing and mainly getting either errors or very complicated data structures that didn't make much sense … it's better to take a rest.

[kellertuer]

I got a small step further, but somehow sometimes I don't understand that the CachedBasis that I assume to have is actually only its internal data. Somewhere this is still confused (mainly also in my head I think).

edfit: Ah its the zero eigenvalue direction of the diagonalising basis – I will check tomorrow afternoon. But the first two bases seem to work.

[kellertuer]

Taking a break was a good idea.

• Product Manifold works for all 3 basis types.
• Power Manifold.
• Vector Bundle
• renamed ArbitraryOrthonormal to Orthonormal, since any non arbitrary will be a subtype (like the Diagonalizing...)
• show methods
• documentation

[codecov]

Codecov Report

Merging #115 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #115   +/-   ##
=======================================
  Coverage   96.87%   96.87%           
=======================================
  Files          49       49           
  Lines        3229     3229           
=======================================
  Hits         3128     3128           
  Misses        101      101           

---

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 64a830f...64a830f. Read the comment docs.

[kellertuer]

I just finished group manifolds (especially identity and) get_vector/get_coordinates. I think this should be fine (up to Mac OS tests now not even starting, but there's already a thread on the travis discourse on that).

[mateuszbaran]

I think this looks OK now.

[kellertuer]

So let's wait for tests and then we can merge.