Conversation
beverlylytle
left a comment
beverlylytle
left a comment
There was a problem hiding this comment.
These look like useful functions when numpy is not available. I strongly encourage adding tests though.
src/compas/utilities/itertools.py
Outdated
| return chain.from_iterable(list_of_lists) | ||
|
|
||
|
|
||
| def unflatten(lst, num): |
There was a problem hiding this comment.
This function seems to be a limited version of reshape, that is, unflatten(l, num) == reshape(l, (len(l)//num), num)). I would suggest for the sake of not cluttering the API that this should be removed.
|
|
||
| def flatten(list_of_lists): | ||
| """Flatten one level of nesting""" | ||
| """Flatten one level of nesting. |
There was a problem hiding this comment.
Since numpy's flatten doesn't flatten just one level, I think this name might be confusing. So I would suggest either renaming this function or replacing it with something that behaves as numpy.flatten does. Eg
def flatten(l):
if not hasattr(l[0], '__len__'):
return l
return flatten(list(chain(*l)))
There was a problem hiding this comment.
i have the feeling this will break a lot of existing code. for example, in case flatten is used on a list of lists of points, the new version will basically return a list of floats, and that is not the case with the previous version.
for point in flatten(list_of_polylines):
# point is a point in the old version
# point is a coordinate in the new version| ---------- | ||
| lst : list | ||
| A list of items. | ||
| shape : int or tuple of ints |
There was a problem hiding this comment.
I don't think it makes sense for shape to be an int. As it is now, that would result in a TypeError, but I don't think it really makes sense to call reshape([1,2,3], 3). It would make sense if the input were something more complicated than a flat list. You could address this by using the version of flatten suggested above and then recursively calling a helper function which does what reshape currently does, but maybe that's more than you need:
def reshape(l, shape):
def helper(l, shape):
if len(shape) == 1:
if len(l) % shape[0] != 0:
raise ValueError()
return [l[i:i + shape[0]] for i in range(0, len(l), shape[0])]
n = reduce(mul, shape[1:])
return [helper(lst[i * n:(i + 1) * n], shape[1:]) for i in range(len(lst) // n)]
shape = (shape,) if instance(shape, int) else shape
l = flatten(l)
if len(l) != reduce(lambda x, y: x * y, shape):
raise ValueError("ValueError: cannot reshape array of size %d into shape %s" % (len(lst), shape))
return helper(l, shape)
There was a problem hiding this comment.
thank you for suggesting even better utility functions ;)
Co-authored-by: beverlylytle <57254617+beverlylytle@users.noreply.github.com>
|
ok, i've implemented changes and added tests. |
Co-authored-by: beverlylytle <57254617+beverlylytle@users.noreply.github.com>
|
@romanarust is there any reason not to merge this? |
src/compas/utilities/itertools.py
Outdated
| if not hasattr(list_of_lists[0], '__len__'): | ||
| return list_of_lists | ||
| return flatten(list(chain(*list_of_lists))) |
There was a problem hiding this comment.
we need to discuss this because i think this is a breaking change...
tomvanmele
left a comment
tomvanmele
left a comment
There was a problem hiding this comment.
just to make sure this is not yet merged by accident
|
We discussed this today in the compas dev meeting. |
|
Ping here again! remove the changes to the "flatten" function, think it is ready now |
|
@romanarust black is complaining |
4 participants
What type of change is this?
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.mdfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke lint).compas.datastructures.Mesh.