Function that generates steps time series from user given values

Question

I created simple function, that generate values of series representing repeating sequence of steps in time. User can define:

step values
width of the steps
how many times the step sequence should repeat
or the size of returned series

If the size is not defined, the size of returned data should be determined by number of repeats.

So the call

steps(2, [1, 2, 3], repeat=2)

should return

[1 1 2 2 3 3 1 1 2 2 3 3]

The function follows

def steps(step_width, values, repeat=1, size=None):
    """
    This function generates steps from given values.

    **Args:**

    * `step_width` - desired width of every step (int)

    * `values` - values for steps (1d array) 

    **Kwargs:**

    * `repeat` - number of step sequence repetions (int), this value is used,
      if the `size` is not defined

    * `size` - size of output data in samples (int), if the `size` is used,
      the `repeat` is ignored.

    **Returns:**

    * array of values representing desired steps (1d array)
    """
    try:
        step_width = int(step_width)
    except:
        raise ValueError('Step width must be an int.') 
    try:
        repeat = int(repeat)
    except:
        raise ValueError('Repeat arg must be an int.') 
    try:
        values = np.array(values)
    except:
        raise ValueError('Values must be a numpy array or similar.') 
    # generate steps
    x = np.repeat(values, step_width)          
    if size is None:
        # repeat according to the desired repetions
        x_full = np.tile(x, repeat)      
    else:
        try:
            repeat = int(repeat)
        except:
            raise ValueError('Repeat arg must be an int.') 
        # repeat till size is reached and crop the data to match size
        repeat = int(np.ceil(size / float(len(x))))
        x_full = np.tile(x, repeat)[:size]
    return x_full

I would appreciate any feedback. Especially I am not sure about effectivity of the error raising as it is implemented right now.

holroy · Accepted Answer · 2017-05-11 19:19:42Z

Some comments related to your code, in addition to the comments made in Jaime's answer:

Lookout for code repetitions – Whenever you catch yourself doing a copy-paste in your code, or typing the same over and over, you should consider either writing loops or methods.

This applies especially to the try...except pattern. You could either use the option provide by Jaime, or the basic extract based upon your code:
```
def validate_int(value, error_msg):
    try:
       return int(value)
    except:
       raise ValueError(error_msg)
```
Repeated conversion of repeat – Why do you repeat the conversion of repeat when size is not None? You've already done this, no need to do it again. Is it a typo, and you should have validated the size number?
Write good comments and docstrings – If I read only the code, I wouldn't understand what your method actually does. Your docstring is also rather large and a little too much space consuming. For alternate version, see code example below.

This is less then half the size of your docstring, and still I've also added some examples to further understand the inner working of the method.

Regarding the comments, I would focus on what we gain from doing the next code statement. For example, see code below.
What do you gain from doing the raise ValueError in this method? – In general when a method is called, you have control on where the numbers come from. If you have calculated them you now they are the proper value already, and steps shouldn't need to verify it.

If they are read either from the command line, or from keyboard input, or possibly through a web request, I would perform the validation closer to the actual retrieval of the value.

For example, if read from keyboard, and you don't validate until this method is called, how would handle it? If you validate when it was typed in, you could enforce the type there, and repeat asking for new input until your type requirements are satisfied.

Another benefit of assuming validation earlier (or closer to the retrieval), is that it would greatly simplify your code. If using the slice trick and repeat calculation presented by Jaime your code would then look like this:
```
def steps(step_width, values, repeat=1, size=None):
    """Generate 'step_width' copies of each of the elements of 'values', and
    either repeat this step sequence 'repeat' times,
    or repeat it until the entire sequences has 'size' elements.

    Two examples:
      step_with(2, [5, 6] -> [5, 5, 6, 6, 5, 5, 6, 6, 5, 5, 6, 6]
      steps(3, [7, 8], size=5) -> [7, 7, 7, 8, 8]
    """

    if size is not None:
        # Calculate how many 'repeat's we need to get
        # at least 'size' elements
        repeat = int(np.ceil(size / float(len(x))))

    return np.tile(np.repeat(values, step_width), repeat)[:size]
```

Jaime · Accepted Answer · 2017-05-11 15:50:33Z

5

Your type checks are distracting, they are probably better extracted to a separate function.

def check_type_or_raise(obj, expected_type, obj_name):
    if not isinstance(obj, expected_type):
        raise TypeError(
            "'{}' must be {}, not {}".format(
                obj_name,
                expected_type.__name,
                obj.__class__.__name__)

Note that this has slightly different behaviour than your code: you would e.g. accept a step_width of 2.5 and convert it to a 2. This is typically not what you want, so I think it is better to raise for such cases.

If you don't mind the conversion, then you probably don't need to wrap your calls to int() in a try and re-raise, as you will already get a nice error, e.g.:

>>> int('abc')
ValueError: invalid literal for int() with base 10: 'abc'

I would also refactor your code to have the output creation happen in a single, common place, after some manipulation of the arguments:

def steps(step_width, values, repeat=1, size=None):
    check_type_or_raise(step_width, int, 'step_width')
    check_type_or_raise(repeat, int, 'repeat')
    values = np.asarray(values)
    if size is not None:
        check_type_or_raise(size, int, 'size')
        # This does rounded up integer division without floating point
        repeat = (size - 1) // (len(values) * step_width) + 1
    return np.tile(np.repeat(values, step_width), repeat)[:size]

answered May 11, 2017 at 15:50

Jaime

6,63721 silver badges30 bronze badges

\$\begingroup\$ What would the last slice do when size=None? \$\endgroup\$

holroy
– holroy

2017-05-11 18:23:27 +00:00
Commented May 11, 2017 at 18:23
\$\begingroup\$ A None in a slice is the same as the default empty, so it will return the full array. \$\endgroup\$

Jaime
– Jaime

2017-05-11 18:29:42 +00:00
Commented May 11, 2017 at 18:29
\$\begingroup\$ Are you saying it'll be the same as [:], which effectively copies the newly created array? \$\endgroup\$

holroy
– holroy

2017-05-11 18:31:30 +00:00
Commented May 11, 2017 at 18:31
\$\begingroup\$ Slicing a numpy array creates a view, not a copy, so it's a cheap operation. You wouldn't want to do that with a normal Python list though. \$\endgroup\$

Jaime
– Jaime

2017-05-11 18:33:47 +00:00
Commented May 11, 2017 at 18:33
\$\begingroup\$ OK! Thanks for educating me on something, which kind of looked like an error (or expensive operation). I'm not all that into numpy, yet. \$\endgroup\$

holroy
– holroy

2017-05-11 18:35:32 +00:00
Commented May 11, 2017 at 18:35

Add a comment |

Stack Exchange Network

Function that generates steps time series from user given values

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Function that generates steps time series from user given values

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions