Too clever?

[bwohlberg]

This is a clever little construction

scico/scico/linop/_stack.py

Line 72 in ddeee4e

[shapes2.append(s) for s in shape]

but is the compactness worth the potential confusion (codefactor does not like it)? The non-obscure way of doing this only requires one extra line, i.e.

for s in shape:
    shapes2.append(s)

[FernandoDavis]

Personally, I like the approach on the first one since it looks clean and compact. I tested the performance and there wasn't much difference perhaps get a test with huge amounts of data and see if there are any performance hits but I doubt it. I think it's readable and neat so it gets a positive vote on my end.

Also, I checked the file and neither shapes nor shapes2 is used after the example, not sure if that's a w.i.p or non-intended but thought id mention it!

[lukepfister]

disagree with codefactor here, and i'd disagree that this is obscure; list comprehensions are the "pythonic way".

i prefer to use list comprehensions like this when it is small enough to remain on one line.

iirc in some contexts list comprehensions are much faster than for loops-- i don't think that is going to be the bottleneck here, though.

all that being said, no real objections to changing it

[bwohlberg]

I don't have very strong feelings about it either, but the most common view within the Python community seems to be that it's unpythonic, e.g. see this discussion.

[lukepfister]

Okay, fair-- my model is typically "making a list? use a list comprehension". the point in the link is that you shouldn't apply functions with side effects inside of a list comprehension, and this is just calling .append. no list is returned.

fair enough, i'm good with changing it

[bwohlberg]

Also, I checked the file and neither shapes nor shapes2 is used after the example, not sure if that's a w.i.p or non-intended but thought id mention it!

I overlooked this until now. It appears that the entire section of code

scico/scico/linop/_stack.py

Lines 70 to 75 in ddeee4e

	for shape in output_shape:
	if is_nested(shape):
	[shapes2.append(s) for s in shape]
	else:
	shapes2.append(shape)
	shapes = output_shape

has no effect on the function output. Perhaps it's some vestige of a previous implementation of the same function that was overlooked for removal? Is there any reason to keep this block of code?

[lukepfister]

@Michael-T-McCann iirc this was your chunk of code

[Michael-T-McCann]

It looks vestigial to me as well, let's remove it.