[WIP] Power manifold #40
Conversation
|
Awesome!! I'm excited to look at this further. Should be able to review near the end of this week. |
Conflicts: Project.toml
Codecov Report
@@ Coverage Diff @@
## master #40 +/- ##
==========================================
+ Coverage 78.45% 82.26% +3.81%
==========================================
Files 14 16 +2
Lines 882 987 +105
==========================================
+ Hits 692 812 +120
+ Misses 190 175 -15
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
So far it looks great, and I love that you can basically make and work with array of points on Manifolds and still leverage all of the speed-ups. I'm wondering if any of that logic can also be applied to ProductManifold.
Would like to see cross overloaded so that if all of the manifolds being multiplied are of the same type, it produces a PowerManifold instead of a ProductManifold. And also the cases of pow by pow, pow by manifold, and manifold by pow.
src/PowerManifold.jl
Outdated
| return PowerManifold{typeof(manifold), Tuple{size...}}(manifold) | ||
| end | ||
|
|
||
| function power_mapreduce(f, op, M::PowerManifold, kwargs::NamedTuple, init, x...) |
There was a problem hiding this comment.
Is there really no way that we can just specialize mapreduce and map for any function whose first argument is PowerManifold? This really requires writing custom code for power manifolds.
This also seems like something that would be useful for product manifolds in general and just specialized for power manifolds.
Or maybe we should package our own functions for
There was a problem hiding this comment.
power_map and power_mapreduce have quite a bit of logic in them that is specific to the power manifold. Product manifolds don't need that since the logic there is much more simple. On the other hand, power_map and power_mapreduce still don't handle all cases and I don't really like that splatting of kwargs. On top of that is the problem of allocation of views on the heap. I don't think there is much reason to additionally restrict ourselves to adding new methods to map and mapreduce.
|
|
||
| generates the power manifold $M^{N_1 \times N_2 \times \dots \times N_n}$. | ||
| """ | ||
| struct PowerManifold{TM<:Manifold, TSize} <: Manifold |
There was a problem hiding this comment.
Should PowerManifold and ProductManifold share a base abstract class? I suspect there will be some shared functionality that can be abstracted out.
There was a problem hiding this comment.
What shared functionality are you thinking about? I don't really see any right now.
By the way, thank you for the review 🙂 .
There was a problem hiding this comment.
I am not sure, whether a common base class is useful. They share that they build a manifold from another one/others, so I also don't see exactly what a common base class might bring as an advantage.
|
The PR is almost ready, there is just one minor change that needs to be done once HybridArrays 0.3.0 is tagged. The code assumes that the power manifold is represented by one big array (any kind should do but Do you think I should make any changes here @sethaxen @kellertuer ? |
|
|
||
| generates the power manifold $M^{N_1 \times N_2 \times \dots \times N_n}$. | ||
| """ | ||
| struct PowerManifold{TM<:Manifold, TSize} <: Manifold |
There was a problem hiding this comment.
I am not sure, whether a common base class is useful. They share that they build a manifold from another one/others, so I also don't see exactly what a common base class might bring as an advantage.
|
I'll merge it soon if there are no significant issues. |
I've made an initial part of the power manifold. I've written
HybridArrays.jlto make them faster but the current version is not benchmarked enough to be sure how much it helps but I'm optimistic about it.