Any way to refactor this?

[John Salerno]

Setting aside, for the moment, the utility of this method or even if
there's a better way, I'm wondering if this is an efficient way to do
it. I admit, there was some copying and pasting, which is what prompts
me to ask the question. Here's the method. (I hope it looks ok, because
it looks really weird for me right now)

def _create_3D_xhatches():
for x in xrange(-axis_length, axis_length + 1):
if x == 0: continue
visual.cylinder(pos=(x,-hatch_length,0),
axis=(0,hatch_length*2,0), radius=hatch_radius)
visual.cylinder(pos=(x,0,-hatch_length),
axis=(0,0,hatch_length*2), radius=hatch_radius)
visual.cylinder(pos=(-hatch_length,x,0),
axis=(hatch_length*2,0,0), radius=hatch_radius)
visual.cylinder(pos=(0,x,-hatch_length),
axis=(0,0,hatch_length*2), radius=hatch_radius)
visual.cylinder(pos=(-hatch_length,0,x),
axis=(hatch_length*2,0,0), radius=hatch_radius)
visual.cylinder(pos=(0,-hatch_length,x),
axis=(0,hatch_length*2,0), radius=hatch_radius)

Since each call to cylinder requires a slightly different format, I
figured I had to do it this way.

Thanks.

 

[James Stroud]

Setting aside, for the moment, the utility of this method or even if
there's a better way, I'm wondering if this is an efficient way to do
it. I admit, there was some copying and pasting, which is what prompts
me to ask the question. Here's the method. (I hope it looks ok, because
it looks really weird for me right now)

def _create_3D_xhatches():
for x in xrange(-axis_length, axis_length + 1):
if x == 0: continue
visual.cylinder(pos=(x,-hatch_length,0),
axis=(0,hatch_length*2,0), radius=hatch_radius)
visual.cylinder(pos=(x,0,-hatch_length),
axis=(0,0,hatch_length*2), radius=hatch_radius)
visual.cylinder(pos=(-hatch_length,x,0),
axis=(hatch_length*2,0,0), radius=hatch_radius)
visual.cylinder(pos=(0,x,-hatch_length),
axis=(0,0,hatch_length*2), radius=hatch_radius)
visual.cylinder(pos=(-hatch_length,0,x),
axis=(hatch_length*2,0,0), radius=hatch_radius)
visual.cylinder(pos=(0,-hatch_length,x),
axis=(0,hatch_length*2,0), radius=hatch_radius)

Since each call to cylinder requires a slightly different format, I
figured I had to do it this way.

Thanks.

Your parameters don't follow an unambiguous pattern. Probably best is to
code the parameters as a set of tuples and iterate over them.

James

 

[Bruno Desthuilliers]

John Salerno a écrit :

Setting aside, for the moment, the utility of this method or even if
there's a better way, I'm wondering if this is an efficient way to do
it. I admit, there was some copying and pasting, which is what prompts
me to ask the question. Here's the method. (I hope it looks ok, because
it looks really weird for me right now)

def _create_3D_xhatches():
for x in xrange(-axis_length, axis_length + 1):
if x == 0: continue
visual.cylinder(pos=(x,-hatch_length,0),
axis=(0,hatch_length*2,0), radius=hatch_radius)
visual.cylinder(pos=(x,0,-hatch_length),
axis=(0,0,hatch_length*2), radius=hatch_radius)
visual.cylinder(pos=(-hatch_length,x,0),
axis=(hatch_length*2,0,0), radius=hatch_radius)
visual.cylinder(pos=(0,x,-hatch_length),
axis=(0,0,hatch_length*2), radius=hatch_radius)
visual.cylinder(pos=(-hatch_length,0,x),
axis=(hatch_length*2,0,0), radius=hatch_radius)
visual.cylinder(pos=(0,-hatch_length,x),
axis=(0,hatch_length*2,0), radius=hatch_radius)

Since each call to cylinder requires a slightly different format, I
figured I had to do it this way.

From a purely efficiency POV, there are some obviously possible
improvements. The first one is to alias visual.cylinder, so you save on
lookup time. The other one is to avoid useless recomputation of
-hatch_length and hatch_length*2.

def _create_3D_xhatches():
cy = visual.cylinder
for x in xrange(-axis_length, axis_length + 1):
if x == 0: continue
b = -hatch_length
c = hatch_length*2
cy(pos=(x, b, 0), axis=(0, c, 0), radius=hatch_radius)
cy(pos=(x, 0, b), axis=(0, 0, c), radius=hatch_radius)
cy(pos=(b, x, 0), axis=(c, 0, 0), radius=hatch_radius)
cy(pos=(0, x, b), axis=(0, 0, c), radius=hatch_radius)
cy(pos=(b, 0, x), axis=(c, 0, 0), radius=hatch_radius)
cy(pos=(0, b, x), axis=(0, c, 0), radius=hatch_radius)

A second step would be to try and generate the permutations by code
instead of writing them all by hand, but I suppose the order is
significant...
There's still an obvious pattern, which is that the position of 'c' in
the axis tuple mirrors the position of 'b' in the pos tuple. There might
be some way to use this to let the computer handle some part of the
repetition...

My 2 cents...

 

[bearophileHUGS]

James Stroud:

Probably best is to code the parameters as
a set of tuples and iterate over them.

I agree. Before:

def _create_3D_xhatches(...pass more parameters here...):
for x in xrange(-axis_length, axis_length + 1):
if x == 0:
continue
visual.cylinder(pos=(x, -hatch_length, 0),
axis=(0, hatch_length*2, 0),
radius=hatch_radius)
visual.cylinder(pos=(x, 0, -hatch_length),
axis=(0, 0, hatch_length*2),
radius=hatch_radius)
visual.cylinder(pos=(-hatch_length, x, 0),
axis=(hatch_length*2, 0, 0),
radius=hatch_radius)
visual.cylinder(pos=(0, x, -hatch_length),
axis=(0, 0, hatch_length*2),
radius=hatch_radius)
visual.cylinder(pos=(-hatch_length, 0, x),
axis=(hatch_length*2, 0, 0),
radius=hatch_radius)
visual.cylinder(pos=(0, -hatch_length, x),
axis=(0, hatch_length*2, 0),
radius=hatch_radius)


And after:

def _create_3D_xhatches(...pass more parameters here...):
hl2 = hatch_length * 2
for x in xrange(-axis_length, axis_length + 1):
if x == 0:
continue
params = [[(x, -hatch_length, 0), (0, hl2, 0)],
[(x, 0, -hatch_length), (0, 0, hl2)]
[(-hatch_length, x, 0), (hl2, 0, 0)],
[(0, x, -hatch_length), (0, 0, hl2)],
[(-hatch_length, 0, x), (hl2, 0, 0)],
[(0, -hatch_length, x), (0, hl2, 0)]]
for pos, axis in params:
visual.cylinder(pos=pos, axis=axis, radius=hatch_radius)

More cleaning can be done.

Bye,
bearophile

 

[John Salerno]

Setting aside, for the moment, the utility of this method or even if
there's a better way, I'm wondering if this is an efficient way to do
it. I admit, there was some copying and pasting, which is what prompts
me to ask the question. Here's the method. (I hope it looks ok, because
it looks really weird for me right now)

Thanks guys. I'm going to look over all this and see what I can do.

 

[John Salerno]

From a purely efficiency POV, there are some obviously possible
improvements. The first one is to alias visual.cylinder, so you save on
lookup time. The other one is to avoid useless recomputation of
-hatch_length and hatch_length*2.

def _create_3D_xhatches():
cy = visual.cylinder
for x in xrange(-axis_length, axis_length + 1):
if x == 0: continue
b = -hatch_length
c = hatch_length*2
cy(pos=(x, b, 0), axis=(0, c, 0), radius=hatch_radius)
cy(pos=(x, 0, b), axis=(0, 0, c), radius=hatch_radius)
cy(pos=(b, x, 0), axis=(c, 0, 0), radius=hatch_radius)
cy(pos=(0, x, b), axis=(0, 0, c), radius=hatch_radius)
cy(pos=(b, 0, x), axis=(c, 0, 0), radius=hatch_radius)
cy(pos=(0, b, x), axis=(0, c, 0), radius=hatch_radius)

Doesn't this call to "cy" still call the function multiple times?

 

[Steven D'Aprano]

Doesn't this call to "cy" still call the function multiple times?

I'm not sure I understand what you mean, but here goes anyway...

Well, yes, but you have to call it six times per loop, with six different
sets of arguments, that's why there are six calls to it. I don't think
there's any way to reduce that (although if there is, the Original Poster
would probably love to hear about it).

Bruno's code has two micro-optimizations. The first is to avoid
looking up visual.cylinder each time (six times the number of loops) and
instead only look it up once. If axis_length is (say) 100, you save 1200
name look-ups of arbitrary complexity.

(Perhaps visual inherits from Klass, which inherits from Spam, which
inherits from Parrot, which inherits from Foo, which inherits from Bar,
which has a method "cylinder". Name look-ups can be time consuming.)

The second is to avoid calculating -hatch_length and hatch_length*2 for
each call, but to calculate them only once per loop. Again, only a
micro-optimization, but arithmetic in Python is more work than in (say) C,
because of the whole object oriented framework. So if you can avoid having
to look up hatch_length.__mul__ repeatedly, you may see a small but
significant time saving.

 

[John Salerno]

Bruno's code has two micro-optimizations. The first is to avoid
looking up visual.cylinder each time (six times the number of loops) and
instead only look it up once.

Oh I see. I was thinking the optimization had something to do with
*calling* the function less often, but that's different from the actual
lookup I suppose.