6
\$\begingroup\$

I'd love to get a review of my code, which I've already submitted as homework. I'd like some guidance as to how to write it more DRY-ly: I wrote the whole round, sqrt and exponentiation twice — how could I write it only once? Ideally I'd want to do it within the same function, it'd be a little ugly to write a function like cube_then_sqrt_then_round_to_2dp(number)

P.S. The variables are intentionally camelCase, I'm aware this is against the recommendations of PEP8

# Created Date: 2022-09-6 17:18:00+08:00
# version ='1.0.0'

import math


# generate_sequences function is used to print a sequence of values rounded to two decimal places starting from the
# start value till it's less than or equal to the maxValue. The sequence is value_n = sqrt(value_(n-1) ^ 3)
# Inputs
#   start (int) - the starting value of the sequence
#   maxValue (int) - the largest value that can be included in the sequence
# Return
#   sequence (list) - a list containing the values of the sequence
def generate_sequence(start, maxValue):
    if type(start) != int or type(maxValue) != int or (not (1 <= start <= 20)) or (not (100000 <= maxValue <= 6000000)):
        return None

    precision = 2
    exponent = 3
    sequence = [round(math.sqrt(start ** exponent), precision)]

    while (currentValue := round(math.sqrt(sequence[-1] ** exponent), precision)) <= maxValue:
        sequence.append(currentValue)

    return sequence


# Test code
print(generate_sequence(2, 6000000))  # Replace value as required for testing

Expected output:

[2.83, 4.76, 10.39, 33.49, 193.81, 2698.14, 140151.17]

\$\endgroup\$
3
  • 1
    \$\begingroup\$ What are you using this code for, and why do you want to perform the rounding? By rounding each number to two decimal places, the round-off errors will compound with each successive element in the sequence. \$\endgroup\$ Sep 7, 2022 at 1:30
  • 1
    \$\begingroup\$ @200_success it's just an assignment question for a unit I'm taking in university, I've already submitted the above code as it produces the desired output already. I perform the rounding just because the question stated I needed to. I'm aware the round-off errors compound and that I should simply round all elements in the list before returning instead of rounding each element when appending. I'm more interested in simply just reducing the repeated code here. \$\endgroup\$ Sep 7, 2022 at 5:11
  • \$\begingroup\$ @200_success the inter-term rounding is indeed a big problem, although some amount of compound error will persist anyway until the algorithm is re-written with the solution to the recurrence relation \$\endgroup\$
    – Reinderien
    Sep 7, 2022 at 23:05

2 Answers 2

12
\$\begingroup\$

Instead of using comments to describe the function, use a doc string. This can then be used by tools, unlike comments.

def generate_sequence(start, maxValue):
    '''
    Return a sequence of values by cubing the previous value, taking the
    square root and rounding to 2 decimal places.

    start - the starting value of the sequence
    maxValue - the largest value that can be included in the sequence
    '''

Don't test argument types like this:

if type(start) != int or type(maxValue) != int or (not (1 <= start <= 20)) or (not (100000 <= maxValue <= 6000000)):
    return None

Instead, we can use type annotations on the function:

def generate_sequence(start: int, maxValue: int):

However, in this case, I see no reason to insist that the inputs have to be integers - surely any numeric type should be acceptable?

The range tests look unhelpful, too - why should a start value greater than 20 result in empty output? If there's a meaningful limit imposed by the problem space, then throw an exception, rather than returning an empty set:

    if start <= 1:
        raise ValueError("start value must be greater than 1")

Instead of separately cubing and taking square root, we can combine them into a single exponentiation, because √x³ ≡ x ** 1.5.

We can avoid the repetition of the calculation by including the start value in our sequence, but removing it when we return:

precision = 2
exponent = 1.5
sequence = [start]

while (currentValue := round((sequence[-1] ** exponent), precision)) <= maxValue:
    sequence.append(currentValue)

return sequence[1:]

As a more advanced step, consider writing an infinite generator of the sequence, and a separate step to truncate it when it reaches the limit value. This allows us to transform the result in other ways, e.g. taking the first N values (making starts of 0 and 1 useful again).

Improved code

This also shows how to add a set of tests, instead of modifying the program for each test. Sorry the function names aren't great - I have writer's block today!

import itertools

def infinite_sequence(value):
    '''
    Generate a sequence of values by cubing the previous value, taking the
    square root and rounding to 2 decimal places.

    value - the starting value of the sequence
    '''

    if value < 0:
        raise ValueError("start value must be positive")

    precision = 2
    exponent = 1.5
    
    while True:
        value = round(value ** exponent, precision)
        yield value

def generate_sequence(start, max_value):
    '''
    Return a list of values by cubing the previous value, taking the
    square root and rounding to 2 decimal places.

    start - the starting value of the sequence
    maxValue - the largest value that can be included in the sequence

    Examples:

    >>> generate_sequence(2, 6000000)
    [2.83, 4.76, 10.39, 33.49, 193.81, 2698.14, 140151.17]

    >>> generate_sequence(2, 2)
    []

    >>> generate_sequence(-2, 0)
    Traceback (innermost last):
    ValueError: start value must be positive

    >>> generate_sequence(0, 6)
    Traceback (innermost last):
    ValueError: start value would result in infinite list

    >>> generate_sequence(1, 6)
    Traceback (innermost last):
    ValueError: start value would result in infinite list
    '''

    if 0 <= start <= 1:
        raise ValueError("start value would result in infinite list")
    
    return list(itertools.takewhile(lambda x: x <= max_value,
                                    infinite_sequence(start)))


if __name__ == '__main__':
    import doctest
    doctest.testmod()
\$\endgroup\$
2
  • \$\begingroup\$ I'm definitely a beginner when it comes to programming so if my criticism is dumb please excuse me. I really appreciate all the tips and hints you've provided up till the portion on the infinite generator. While I understand it makes the code more general and concise overall, my view is that its benefits are outweighed by how much less readable the code is now. Perhaps this is a limitation of my own creation due to having used imperative languages all my life and only having dipped my toes into functional programming languages/styles \$\endgroup\$ Sep 7, 2022 at 14:35
  • 1
    \$\begingroup\$ No problem - I did describe it as "more advanced"! Personally, I find the generator version much simpler (we just alternate between computing the next value and yielding it). Having a function to turn that into a finite list does indeed seem complex (though it's still much smaller than its tests!), and probably not worth it if that's the only thing we want to do with the result. In most cases, we wouldn't write that, though - we'd just use the generator directly (e.g. for value in infinite_sequence(): if value > 6000000: break; else: print(value)). \$\endgroup\$ Sep 7, 2022 at 14:49
3
\$\begingroup\$

Don't include a Created Date. That's what source control is for.

It's not an awful idea to do type checking, though you should prioritise PEP484-hinting your signature before doing runtime checking. Both hinting and runtime checking can check against Real. Don't if type() !=; use isinstance instead. Don't return None on error; raise an exception (in this case TypeError) instead.

The inter-term rounding is a bad requirement. Since this is an exponential series, the error compounding occurs very quickly. If you want to round on output, fine; but don't round between terms. Any half-decent prof would accept this explanation, particularly given the recurrence properties below.

This is a recurrence relation that is easily solvable. Once you solve it, several previously impossible strategies become possible:

  • No more looping. Vectorise with Numpy.
  • No indeterminate sequence end. The sequence end is determined analytically.
  • No compound error.

Suggested

from numbers import Real
import numpy as np


def generate_sequence(start: Real, max_value: Real) -> np.ndarray:
    if not isinstance(start, Real) or not isinstance(max_value, Real):
        raise TypeError()

    iend = np.log(np.log(max_value) / np.log(start))/np.log(1.5)
    return start**(1.5**np.arange(1, iend))


def test() -> None:
    result = generate_sequence(2, 6e6)
    assert np.allclose(
        result, (
                 2.82843,
                 4.75683,
                10.37472,
                33.41676,
               193.17305,
              2684.84856,
            139116.83594,
        )
    )


if __name__ == '__main__':
    test()
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.