Skip to main content
added 271 characters in body
Source Link
Graipher
  • 41.1k
  • 7
  • 69
  • 133
  • You can go one step further and create a primes_in_range function that does the dropping for you. This might actually become a bottleneck at some point, (if a is large and a >> b - a), but at that point you only need to change the implementation of one function.

  • Your reference_slice is always the same length, but if you want to add the next element you need to resort to list slicing and list addition, both of which might create a copy. Instead, you can use collections.deque with the optional argument maxlen. If you append another element, the first one will be automatically pushed out.

  • Similarly, you can avoid the slice in the calculation of the differences by just including the first element. You just have to add a 0 as first element in the correct differences list. Alternatively you could just define a diff function and adjust the correct differences to be relative to the previous elements instead of the first.

  • You should limit your try block as much as possible. This increases readability on one hand, and reduces unwanted exceptions being caught on the other (not such a big risk here, but true in general).

  • But even better is to not need it at all. A for loop consumes all elements of an iterable, which is exactly what we need here. This way we don't even need to special case the first five elements, because if the list is too short, the pattern cannot match. Alternatively, you could implement a windowed(it, n) function, which is what I have done below.

  • You should try to avoid magical values. In your code there are two. The first is the length of the pattern and the second is the pattern itself. Fortunately, the former is already given by the latter. I would make the pattern an argument of the function, which makes it also more general. At this point a less generic name might also be needed (although the name I propose below is maybe not ideal either).

  • You should protect the code executing your functions with a if __name__ == "__main__"; guard to allow importing from this script without it being run.

  • Your reference_slice is always the same length, but if you want to add the next element you need to resort to list slicing and list addition, both of which might create a copy. Instead, you can use collections.deque with the optional argument maxlen. If you append another element, the first one will be automatically pushed out.

  • Similarly, you can avoid the slice in the calculation of the differences by just including the first element. You just have to add a 0 as first element in the correct differences list. Alternatively you could just define a diff function and adjust the correct differences to be relative to the previous elements instead of the first.

  • You should limit your try block as much as possible. This increases readability on one hand, and reduces unwanted exceptions being caught on the other (not such a big risk here, but true in general).

  • But even better is to not need it at all. A for loop consumes all elements of an iterable, which is exactly what we need here. This way we don't even need to special case the first five elements, because if the list is too short, the pattern cannot match. Alternatively, you could implement a windowed(it, n) function, which is what I have done below.

  • You should try to avoid magical values. In your code there are two. The first is the length of the pattern and the second is the pattern itself. Fortunately, the former is already given by the latter. I would make the pattern an argument of the function, which makes it also more general. At this point a less generic name might also be needed (although the name I propose below is maybe not ideal either).

  • You should protect the code executing your functions with a if __name__ == "__main__"; guard to allow importing from this script without it being run.

  • You can go one step further and create a primes_in_range function that does the dropping for you. This might actually become a bottleneck at some point, (if a is large and a >> b - a), but at that point you only need to change the implementation of one function.

  • Your reference_slice is always the same length, but if you want to add the next element you need to resort to list slicing and list addition, both of which might create a copy. Instead, you can use collections.deque with the optional argument maxlen. If you append another element, the first one will be automatically pushed out.

  • Similarly, you can avoid the slice in the calculation of the differences by just including the first element. You just have to add a 0 as first element in the correct differences list. Alternatively you could just define a diff function and adjust the correct differences to be relative to the previous elements instead of the first.

  • You should limit your try block as much as possible. This increases readability on one hand, and reduces unwanted exceptions being caught on the other (not such a big risk here, but true in general).

  • But even better is to not need it at all. A for loop consumes all elements of an iterable, which is exactly what we need here. This way we don't even need to special case the first five elements, because if the list is too short, the pattern cannot match. Alternatively, you could implement a windowed(it, n) function, which is what I have done below.

  • You should try to avoid magical values. In your code there are two. The first is the length of the pattern and the second is the pattern itself. Fortunately, the former is already given by the latter. I would make the pattern an argument of the function, which makes it also more general. At this point a less generic name might also be needed (although the name I propose below is maybe not ideal either).

  • You should protect the code executing your functions with a if __name__ == "__main__"; guard to allow importing from this script without it being run.

added 14 characters in body
Source Link
Graipher
  • 41.1k
  • 7
  • 69
  • 133
from collections import deque
from itertools import dropwhile, islice


def prime_sieve(n):
    ...

def primes_in_range(a, b):
    yield from dropwhile(lambda x: x < a, prime_sieve(b))

def diff(it):
    it = iter(it)
    previous = next(it)
    for x in it:
        yield x - previous
        previous = x

def windowed(it, n):
    it = iter(it)
    d = deque(islice(it, n), maxlen=n)
    assert len(d) == n
    for x in it:
        yield tuple(d)
        d.append(x)

def find_primes_matching_pattern_below(diff_pattern, n):
    min_value = (n - 48) / 6   # A comment explaining this bound
    primes = dropwhileprimes_in_range(lambda v: v < min_value, prime_sieve(n))
    for candidate in windowed(primes, len(diff_pattern) + 1):
        if tuple(diff(candidate)) == diff_pattern:
            yield candidate


if __name__ == "__main__":
    n = 2000000
    # from (4, 6, 10, 12, 16) relative to the first element
    diff_pattern = (4, 2, 4, 2, 4)
    print(next(find_primes_matching_pattern_below(diff_pattern, n)))

For the given testcase, your code takes 741 ms ± 34.3 ms, while this code takes 146 ms ± 3.77 ms (using the numpy prime sieve from the answer by @OscarSmith) on my machine.

from collections import deque
from itertools import dropwhile, islice


def prime_sieve(n):
    ...

def diff(it):
    it = iter(it)
    previous = next(it)
    for x in it:
        yield x - previous
        previous = x

def windowed(it, n):
    it = iter(it)
    d = deque(islice(it, n), maxlen=n)
    for x in it:
        yield tuple(d)
        d.append(x)

def find_primes_matching_pattern_below(diff_pattern, n):
    min_value = (n - 48) / 6   # A comment explaining this bound
    primes = dropwhile(lambda v: v < min_value, prime_sieve(n))
    for candidate in windowed(primes, len(diff_pattern) + 1):
        if tuple(diff(candidate)) == diff_pattern:
            yield candidate


if __name__ == "__main__":
    n = 2000000
    # from (4, 6, 10, 12, 16) relative to the first element
    diff_pattern = (4, 2, 4, 2, 4)
    print(next(find_primes_matching_pattern_below(diff_pattern, n)))

For the given testcase, your code takes 741 ms ± 34.3 ms, while this code takes 146 ms ± 3.77 ms (using the numpy prime sieve from the answer by @OscarSmith).

from collections import deque
from itertools import dropwhile, islice


def prime_sieve(n):
    ...

def primes_in_range(a, b):
    yield from dropwhile(lambda x: x < a, prime_sieve(b))

def diff(it):
    it = iter(it)
    previous = next(it)
    for x in it:
        yield x - previous
        previous = x

def windowed(it, n):
    d = deque(islice(it, n), maxlen=n)
    assert len(d) == n
    for x in it:
        yield tuple(d)
        d.append(x)

def find_primes_matching_pattern_below(diff_pattern, n):
    min_value = (n - 48) / 6   # A comment explaining this bound
    primes = primes_in_range(min_value, n)
    for candidate in windowed(primes, len(diff_pattern) + 1):
        if tuple(diff(candidate)) == diff_pattern:
            yield candidate


if __name__ == "__main__":
    n = 2000000
    # from (4, 6, 10, 12, 16) relative to the first element
    diff_pattern = (4, 2, 4, 2, 4)
    print(next(find_primes_matching_pattern_below(diff_pattern, n)))

For the given testcase, your code takes 741 ms ± 34.3 ms, while this code takes 146 ms ± 3.77 ms (using the numpy prime sieve from the answer by @OscarSmith) on my machine.

added 7 characters in body
Source Link
Graipher
  • 41.1k
  • 7
  • 69
  • 133

I agree with the tips given in the answer by @OscarSmith regarding the prime sieve. In addition, here are a few more comments on how to make the rest of your code slightly better.

All of these changes make your function doing the actual work a lot shorter and IMO more readable. The functions I defined below are also nice things to have in you toolkit, since they are generally applicable and not only for this specific usecase.

(Untested code)

from collections import deque
from itertools import dropwhile, islice


def prime_sieve(n):
    ...

def diff(it):
    it = iter(it)
    previous = next(it)
    for x in it:
        yield x - previous
        previous = x

def windowed(it, n):
    it = iter(it)
    d = deque(islice(it, n), maxlen=n)
    for x in it:
        yield tuple(d)
        d.append(x)

def find_primes_matching_pattern_below(diff_pattern, n):
    min_value = (n - 48) / 6   # A comment explaining this bound
    primes = dropwhile(lambda v: v < min_value, prime_sieve(n))
 
    for candidate in windowed(primes, len(diff_pattern) + 1):
        if tuple(diff(candidate)) == diff_pattern:
            yield candidate
         

if __name__ == "__main__":
    n = 2000000
    diff_pattern = (4, 2, 4, 2, 4)  # from (4, 6, 10, 12, 16) relative to the first element
    diff_pattern = (4, 2, 4, 2, 4)
    print(next(find_primes_matching_pattern_below(diff_pattern, n)))

For the given testcase, your code takes 741 ms ± 34.3 ms, while this code takes 146 ms ± 3.77 ms (using the numpy prime sieve from the answer by @OscarSmith).

I agree with the tips given in the answer by @OscarSmith. In addition, here are a few more comments on how to make the rest of your code slightly better.

All of these changes make your function doing the actual work a lot shorter and IMO more readable. The functions I defined below are also nice things to have in you toolkit, since they are generally applicable and not only for this specific usecase.

(Untested code)

from collections import deque
from itertools import dropwhile, islice


def prime_sieve(n):
    ...

def diff(it):
    it = iter(it)
    previous = next(it)
    for x in it:
        yield x - previous
        previous = x

def windowed(it, n):
    it = iter(it)
    d = deque(islice(it, n), maxlen=n)
    for x in it:
        yield tuple(d)
        d.append(x)

def find_primes_matching_pattern_below(diff_pattern, n):
    min_value = (n - 48) / 6   # A comment explaining this bound
    primes = dropwhile(lambda v: v < min_value, prime_sieve(n))
 
    for candidate in windowed(primes, len(diff_pattern) + 1):
        if diff(candidate) == diff_pattern:
            yield candidate
        
if __name__ == "__main__":
    n = 2000000
    diff_pattern = (4, 2, 4, 2, 4)  # from (4, 6, 10, 12, 16) relative to the first element
    print(next(find_primes_matching_pattern_below(diff_pattern, n)))

I agree with the tips given in the answer by @OscarSmith regarding the prime sieve. In addition, here are a few more comments on how to make the rest of your code slightly better.

All of these changes make your function doing the actual work a lot shorter and IMO more readable. The functions I defined below are also nice things to have in you toolkit, since they are generally applicable and not only for this specific usecase.

from collections import deque
from itertools import dropwhile, islice


def prime_sieve(n):
    ...

def diff(it):
    it = iter(it)
    previous = next(it)
    for x in it:
        yield x - previous
        previous = x

def windowed(it, n):
    it = iter(it)
    d = deque(islice(it, n), maxlen=n)
    for x in it:
        yield tuple(d)
        d.append(x)

def find_primes_matching_pattern_below(diff_pattern, n):
    min_value = (n - 48) / 6   # A comment explaining this bound
    primes = dropwhile(lambda v: v < min_value, prime_sieve(n))
    for candidate in windowed(primes, len(diff_pattern) + 1):
        if tuple(diff(candidate)) == diff_pattern:
            yield candidate
 

if __name__ == "__main__":
    n = 2000000
    # from (4, 6, 10, 12, 16) relative to the first element
    diff_pattern = (4, 2, 4, 2, 4)
    print(next(find_primes_matching_pattern_below(diff_pattern, n)))

For the given testcase, your code takes 741 ms ± 34.3 ms, while this code takes 146 ms ± 3.77 ms (using the numpy prime sieve from the answer by @OscarSmith).

Source Link
Graipher
  • 41.1k
  • 7
  • 69
  • 133
Loading