MAINT: avoid ambiguous list indexing, closes #1413 #1420
Conversation
| indices = (slice(None),) * 2 | ||
| indices += tuple(n // 2 for n in self.space.shape[2:]) | ||
| indices = tuple( | ||
| [slice(None)] * 2 + [n // 2 for n in self.space.shape[2:]] |
There was a problem hiding this comment.
No need to create a list, the generator is enough
There was a problem hiding this comment.
I can't do * 2 or + with a generator, that would require chaining and stuff. This is the simplest implementation, I think.
There was a problem hiding this comment.
Use parentheses
Edit: I'm stupid, nope.
odl/util/numerics.py
Outdated
| @@ -742,13 +748,15 @@ def _apply_padding(lhs_arr, rhs_arr, offset, pad_mode, direction): | |||
| bdry_slc_l[axis] = left_slc | |||
| bdry_slc_r = list(working_slc) | |||
| bdry_slc_r[axis] = right_slc | |||
| bdry_slc_l, bdry_slc_r = map(tuple, [bdry_slc_l, bdry_slc_r]) | |||
There was a problem hiding this comment.
Just write one per line
bdry_slc_l = tuple(bdry_slc_l)
bdry_slc_r = tuple(bdry_slc_r)
Slight DRY violation, but it makes the code much more readable and maintainable for non-pythonistas.
There was a problem hiding this comment.
I'll do the cases with 2, but the ones with 4 just feel wrong to put in 4 lines. If the functional version is not great, I can also do list comprehension:
# Old
lhs_slc_l, rhs_slc_l, lhs_slc_r, rhs_slc_r = map(
tuple, [lhs_slc_l, rhs_slc_l, lhs_slc_r, rhs_slc_r])
# New
lhs_slc_l, rhs_slc_l, lhs_slc_r, rhs_slc_r = [
tuple(lst) for lst in [lhs_slc_l, rhs_slc_l, lhs_slc_r, rhs_slc_r]
]Or I just write a little inline helper
def to_tuple(*args):
return tuple(tuple(arg) for arg in args)There was a problem hiding this comment.
For 3 or more, I'm fine with the map approach, its just that for 2 it's barely any reduction.
kohr-h
force-pushed
the
issue-1413__tuple_indexing
branch
from
09d7142
to
cc124dc
Compare
|
Alright, merging after CI. |
|
Build issue with anaconda and python 2.7 here as well, merging anyway. |
We had a few places where we explicitly created index lists that shouldn't be lists. The unit tests unearthed a whole bunch of them, but there might be more.
I guess we can go ahead with this (assuming the tests go green) and fix other places that trigger warnings later on.