Make ArrayPartition default instead of ProductRepr

[mateuszbaran]

Resolves one of the issues raised in #609 .

This should be only slightly breaking so I don't think an actual breaking release is necessary.

[codecov]

Codecov Report

Merging #612 (fdd27c8) into master (8f95a93) will increase coverage by 0.00%.
The diff coverage is 96.42%.

@@           Coverage Diff           @@
##           master     #612   +/-   ##
=======================================
  Coverage   99.00%   99.00%           
=======================================
  Files         104      104           
  Lines       10039    10047    +8     
=======================================
+ Hits         9939     9947    +8     
  Misses        100      100           

Impacted Files	Coverage Δ
src/product_representations.jl	96.87% <88.88%> (+0.17%)	⬆️
src/groups/product_group.jl	100.00% <100.00%> (ø)
src/groups/semidirect_product_group.jl	100.00% <100.00%> (ø)
src/groups/special_euclidean.jl	99.60% <100.00%> (+<0.01%)	⬆️
src/manifolds/Circle.jl	100.00% <100.00%> (ø)
src/manifolds/ProductManifold.jl	96.46% <100.00%> (ø)
src/manifolds/VectorBundle.jl	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[kellertuer]

How breaking this is is maybe hard to estimate?
But we could introduce a default function (yet again) for this as well?

And we could also deprecate ProductRepr?

[mateuszbaran]

How breaking this is is maybe hard to estimate?
But we could introduce a default function (yet again) for this as well?

.parts needs to be changed to .x, if someone uses that, so not much work to fix it. What kind of default function do you mean here?

And we could also deprecate ProductRepr?

We could. Probably the biggest benefit would be reducing testing time in Manifolds 0.9 (when it would be removed).

[kellertuer]

You changed quite some code for the switch, so a default data type might be helpful in case we want to switch again?

I think in favour of ArrayPartition we should deprecate ProductRepr (and remove it in 0.9), since I do not see much gain in inventing our own data type if probably better tested and maybe more efficient others do exist.

[mateuszbaran]

Most code changes is either adapting tests or adding some missing functionality to ArrayPartition. Actually changing the default is something like replacing ProductRepr with ArrayPartition in a few places.

I guess the only way to switch defaults would be encoding it in the manifold?

ArrayPartition is generally a better choice. I don't quite like the way it's printed and the linear indexing seems like a hacky and dangerous feature but it's more compatible thanks to it.

[kellertuer]

I just feel it is maybe a bit beyond what Manifolds.jl should provide, if it exists - unless there is a really good reason for ProductRepr.

The printing could be a PR to the original package then?
About the internals, you are more experienced than me.

[mateuszbaran]

I just feel it is maybe a bit beyond what Manifolds.jl should provide, if it exists - unless there is a really good reason for ProductRepr.

That's exactly the direction we are going in here -- removing ProductRepr from Manifolds.jl.

The printing could be a PR to the original package then?

Maybe I'll try then.

[mateuszbaran]

@kellertuer I think this is ready 🙂

[mateuszbaran]

I remember reworking Circle some time ago to make it accept one-element vectors as points and tangent vectors but the current implementation is a bit conflicted about it.

[kellertuer]

I hope we did not loose any of these in merges? I had in mind Circle works for both, yes.

[mateuszbaran]

It was a bit incorrect but I haven't checked git blame to see why.