Python custom generator of object not efficient

Question

Let's consider the following class Signal which defines a multiphasic signal:

class Signal:
 def __init__(self, fq, phases):
 self.fq = fq
 self.phases = phases

I created a function that generates Signal to test in another function.

# -*- coding: utf-8 -*-
import itertools
def duo_SBs_to_test(SB1, SB2, frequency_step = 1, shift_step = 50, max_shift = 20000, permutations = True):
 upper_frequency = frequency_step + 1/(2*max(sum(SB1.phases), sum(SB2.phases)))*1000000
 if SB1.fq <= 100 and SB2.fq <= 100:
 r1 = 0.1
 r2 = 0.1
 upper1 = SB1.fq + r1*SB1.fq + frequency_step
 upper2 = SB2.fq + r2*SB2.fq + frequency_step
 elif SB1.fq <= 100 and SB2.fq > 100:
 r1 = 0.1
 r2 = 0.2
 upper1 = SB1.fq + r1*SB1.fq + frequency_step
 upper2 = SB2.fq + r2*SB2.fq + frequency_step
 if upper2 > upper_frequency:
 upper2 = upper_frequency
 elif SB1.fq > 100 and SB2.fq <= 100:
 r1 = 0.2
 r2 = 0.1
 upper1 = SB1.fq + r1*SB1.fq + frequency_step
 upper2 = SB2.fq + r2*SB2.fq + frequency_step
 if upper1 > upper_frequency:
 upper1 = upper_frequency
 else: 
 r1 = 0.2
 r2 = 0.2
 upper1 = SB1.fq + r1*SB1.fq + frequency_step
 upper2 = SB2.fq + r2*SB2.fq + frequency_step
 if upper1 > upper_frequency:
 upper1 = upper_frequency
 if upper2 > upper_frequency:
 upper2 = upper_frequency
 for s, f1, f2 in itertools.product(range(0, max_shift+shift_step, shift_step), 
 range(int(SB1.fq - r1*SB1.fq), int(upper1), frequency_step), 
 range(int(SB2.fq - r2*SB2.fq), int(upper2), frequency_step)):
 yield (Signal(f1, SB1.phases), Signal(f2, SB2.phases), s)
 if permutations:
 yield (Signal(f2, SB2.phases), Signal(f1, SB1.phases), s)

Surprisingly, the performance isn't great. It's my first time creating a generator, and I guess I missed something.

%timeit res = [elt for elt in duo_SBs_to_test(SB1, SB3, frequency_step = 1, 
shift_step = 50, max_shift = 20000, permutations = True)]
829 ms ± 11.2 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
%timeit res = [elt for elt in duo_SBs_to_test(SB1, SB3, frequency_step = 1, 
shift_step = 50, max_shift = 20000, permutations = True)]
842 ms ± 13.7 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
%timeit for elt in duo_SBs_to_test(SB1, SB3, frequency_step = 1, 
shift_step = 50, max_shift = 20000, permutations = True): continue
765 ms ± 5.81 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
%timeit for elt in duo_SBs_to_test(SB1, SB3, frequency_step = 1, 
shift_step = 50, max_shift = 20000, permutations = True): continue
768 ms ± 5.66 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

The former function I was using was storing frequency_to_test in a list, shifts in a numpy array before running the itertools.product() and storing in a list the Signals_to_test. Yet, it was just as fast...

EDIT:

SB1 = Signal(50, [300, 50, 900])
SB3 = Signal(80, [300, 50, 900])

Thanks for the tips :)

EDIT2: Next step:

def compute(SB1, SB2, frequency_step = 1, shift_step = 50, max_shift = 20000, permutations = True):
 result = list()
 for duo_generated in duo_SBs_to_test(SB1, SB2, frequency_step, shift_step, max_shift, permutations):
 if not condition(duo_generated):
 continue
 else:
 result.append(duo_generated)
 return result

condition() is a function returning a boolean.

• 301_Moved_Permanently: When you yield (SB(...), SB(...)) do you mean Signal? Otherwise, please give the definition of SB. Also, could you share the definition of SB1 and SB3 so we can recreate your timings.
• Mathieu: @MathiasEttinger Yes, what I need to yield is a list or tuple of object Signals. See Edit for SB1 and SB3. I've taken out the amplitude of the object which is not relevant to this problem. The timing might still differ slightly as my object Signalis a bit different
• 301_Moved_Permanently: That doesn't make much sense. If you want to yield Signal objects why do you yield these cryptic SB objects. Besides, the signature of the constructor does not match (SB take 3 parameters, but Signal only 2).
• Mathieu: @MathiasEttinger My bad it's fixed! I've just noticed the typo error... SBi are object Signal.
• 301_Moved_Permanently: Just removing stuff to make it match what you’re saying isn't going to help. Removing that s parameter from the constructor means you could as well remove it from the product and get way better performances... Can't you paste your real code instead?

Answer 1

Your ifs structure is needlessly complicated. They only set r1 and r2 based on simple rules (r1 [resp. r2] is 0.1 if SB1.fq [resp. SB2.fq] is lower than 100 and 0.2 otherwise) and compute upper1 and upper2 using the same formula. So you can use the simpler:

r1 = 0.1 if SB1.fq <= 100 else 0.2
r2 = 0.1 if SB2.fq <= 100 else 0.2
upper1 = (1 + r1) * SB1.fq + frequency_step
upper2 = (1 + r2) * SB2.fq + frequency_step

You can also use min to make these values not go over upper_frequency. But this is still some redundancies, especially given the fact that, later, you use r1 & SB1 and r2 & SB2 in the same kind of pattern in your ranges. So better extract out a function:

def frequency_range(SB, step, higher_frequency):
 r = 0.1 if SB.fq <= 100 else 0.2
 high = (1 + r) * SB.fq + step
 low = (1 - r) * SB.fq
 return range(int(low), int(min(high, higher_frequency)))

And the main function is now:

def duo_SBs_to_test(SB1, SB2, frequency_step=1, shift_step=50, max_shift=20000, permutations=True):
 max_phases = max(sum(SB1.phases), sum(SB2.phases))
 upper_frequency = frequency_step + 1/(2 * max_phases) * 1000000
 parameters = itertools.product(
 range(0, max_shift + shift_step, shift_step),
 frequency_range(SB1, frequency_step, upper_frequency),
 frequency_range(SB2, frequency_step, upper_frequency))
 for s, f1, f2 in parameters:
 yield Signal(f1, SB1.phase), Signal(f2, SB2.phase), s
 if permutations:
 yield Signal(f2, SB2.phase), Signal(f1, SB1.phase), s

Now the only little change that I can think of that could improve performances is to avoid testing permutations at each iteration. So you could perform two "radicaly" different iterations depending on the value of permutations:

def duo_SBs_to_test(SB1, SB2, frequency_step=1, shift_step=50, max_shift=20000, permutations=True):
 max_phases = max(sum(SB1.phases), sum(SB2.phases))
 upper_frequency = frequency_step + 1/(2 * max_phases) * 1000000
 parameters = itertools.product(
 range(0, max_shift + shift_step, shift_step),
 frequency_range(SB1, frequency_step, upper_frequency),
 frequency_range(SB2, frequency_step, upper_frequency))
 if permutations:
 for s, f1, f2 in parameters:
 yield Signal(f1, SB1.phase), Signal(f2, SB2.phase), s
 yield Signal(f2, SB2.phase), Signal(f1, SB1.phase), s
 else:
 for s, f1, f2 in parameters:
 yield Signal(f1, SB1.phase), Signal(f2, SB2.phase), s

---

Now, as regard to your next step, you’re basically just reinventing filter. Just use that instead:

def compute(SB1, SB2, frequency_step = 1, shift_step = 50, max_shift = 20000, permutations = True):
 return list(filter(condition, duo_SBs_to_test(SB1, SB2, frequency_step, shift_step, max_shift, permutations))

And if that compute function is called in a for loop, you can also remove the need to convert it to a list.

• Mathieu: Thanks. Sorry for the bumpy start with the typos mistakes. I will test this tomorrow. Thanks for pointing out the filter() function. I was not aware of it. I'm using several conditions to validate an input, thus I might need to filter several time. I will test the efficiency tomorrow.