add flatten() method and test#112
Conversation
tBuLi
left a comment
There was a problem hiding this comment.
Thanks for this PR! Definitelly something we want to have. I just have some comments.
|
|
||
| # Get shapes of all values | ||
| shapes = [] | ||
| for v in self._values: |
There was a problem hiding this comment.
I would prefer a list comprehension, one concept one line.
|
|
||
| # Broadcast each value to the common shape | ||
| broadcasted_values = [] | ||
| for v in self._values: |
|
|
||
| return self.fromkeysvalues(self.algebra, keys=self._keys, values=broadcasted_values) | ||
|
|
||
| def flatten(self): |
There was a problem hiding this comment.
This should still return a multivector. For example, for a multivector of shape (4, N, M) I would expect flatten to return a multivector of shape (4, M*N). Since multivectors are still iterable there is no difference when using flatten for loops but this also still allows you to do GA operatons on it efficiently.
Moreover, the datatype of _values for the new array should be the same of the old one, when possible.
| :return: List of flattened multivectors. | ||
| """ | ||
| mv = self.broadcast() | ||
| return [mv[i] for i in np.ndindex(mv.shape[1:])] |
There was a problem hiding this comment.
I would rename i to indices because i suggests a single int.
Moreover, we can easily add a depth argument to this method, which should default to a full flatten like numpy does:
def flatten(self, depth=-1):
mv = self.broadcast()
return [mv[i] for i in np.ndindex(mv.shape[1:(len(mv.shape) if depth is None else depth+1)])]Also add a test for this, I might have an of by one error :P.
This in addition to the previous comment, so this function will still have to change more.
|
|
||
| def test_flatten(): | ||
| alg = Algebra(4) | ||
| shape = (len(tuple(alg.indices_for_grade(2))), 3, 4) |
There was a problem hiding this comment.
I dn't think you need to cast to tuple to get the right len?
| else: | ||
| raise NotImplementedError | ||
|
|
||
| def broadcast(self) -> "MultiVector": |
There was a problem hiding this comment.
Upon reflection I am not sure if we should add this on the mv. At least not as it stands, because in the end it is numpy specific. I guess figuring out the the broadcast shape with
broadcast_shape = np.broadcast_shapes(*shapes)will work for e.g. pytorch as well, but these lines are numpy specific:
if hasattr(v, 'shape'):
# It's array-like, use numpy broadcasting
broadcasted_values.append(np.broadcast_to(v, broadcast_shape))
else:
# It's a scalar, broadcast to the target shape
broadcasted_values.append(np.full(broadcast_shape, v))which should be made flexible by adding broadcast_to and full as keyword arguments with their default set to the current numpy ones.
But the reason I am doubtful if this is needed all together, is because in the near future I intend to add more advanced typing to kingdon. So for example, to make a point in 2d pga you might currently do
alg = ALgebra.fromname("2DPGA")
xvals = np.random.random(10)
yvals = np.random.random(10)
alg.vector(e0=1, e1=xvals, e2=xvals).dual()and in this scenario the broadcast function would make a lot of sense.
But in the near future you would instead do
alg.point(xvals, yvals)where the homogenous 1 is absorbed into the type. So in this scenario, I think it is fair to leave the broadcasting responsibility entirely with the user.
i am not sure the correct way to do this in full generallity, but it was helpful for plotting arrays-of mv's.