5
\$\begingroup\$

I'm looking for a review on my code. I am also looking for ways to transform this function into something more Pythonic. I'm fairly new to Python, so any advice would be appreciated.

def get_rsi(self, period):
    rsi = []

    for i in xrange(1, len(self.hist_d) - period + 1):
        gains = 0.0
        losses = 0.0

        for j in xrange(i, i + period):
            diff = self.hist_d[j][2] - self.hist_d[j - 1][2]

            if diff > 0:
                gains += diff
            elif diff < 0: 
                losses += abs(diff)

        rsi.append(round(100 - (100 / (1 + gains / losses)), 2))

    return rsi
\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

Your code is already pretty Pythonic.

There is a bug in your code. If no losses were reported in that window then losses == 0.0 which when you append to rsi will throw a ZeroDivisionError.

There are two suggestions I would make:

  1. Use zip (or izip if possible)

    The zip function takes two iterables and combines them. It essentially returns:

    >>>zip([1,2,3], ['a','b','c'])
    [(1, 'a'), (2, 'b'), (3, 'c')]
    

    We can use this instead of directly indexing through j and j-1.

  2. If-Else

    Currently your if-statement looks like this:

    if diff > 0:
        gains += diff
    elif diff < 0: 
        losses += abs(diff)
    

    I would recommend making the elif into a simple else. This is because anything that the elif won't catch is if diff == 0 and since losses == losses + 0, it won't effect losses. Also, you remove a comparision using a simple else.

With these suggestions taken into account (plus the simple bug fix), we can refactor your code a little bit:

def get_rsi(self, period):
    rsi=[]
    for i in range(len(self.hist_d)-period):
        gains = 0.0
        losses = 0.0

        window = self.hist_d[i:i+period+1]
        for year_one, year_two in zip(window, window[1:]):
            diff = year_two - year_one

            if diff > 0:
                gains += diff
            else:
                losses += abs(diff)

        # Check if `losses` is zero. If so, `100/(1 + RS)` will practically be 0.
        if not losses:
            rsi.append(100.00)
        else:
            rsi.append(round(100 - (100 / (1 + gains / losses)), 2))

    return rsi
\$\endgroup\$
0
5
\$\begingroup\$

It looks like you want a moving window of length period over self.hist_d (and then a moving window of length 2 over each of those windows, to get pairs of consecutive years). An efficient way of doing that is provided in the old version of the itertools documentation:

from itertools import islice, izip

def window(seq, n=2):
    "Returns a sliding window (of width n) over data from the iterable"
    "   s -> (s0,s1,...s[n-1]), (s1,s2,...,sn), ...                   "
    it = iter(seq)
    result = tuple(islice(it, n))
    if len(result) == n:
        yield result    
    for elem in it:
        result = result[1:] + (elem,)
        yield result

I heard of this via this SO question, where there are also other options for the same task.

You can then use this to process your results:

def get_rsi(self, period):
    for series in window(self.hist_d, period):
        gains = losses = 0.0
        for year1, year2 in window(series): # or 'izip(series, series[1:])'
            diff = year2[2] - year1[2]
            ...
\$\endgroup\$
5
  • \$\begingroup\$ He doesn't want a static second window of size two. Its of size period. \$\endgroup\$ Commented May 22, 2014 at 14:36
  • \$\begingroup\$ @DarinDouglass the inner, length-2 window in my code is for diff = self.hist_d[j][2] - self.hist_d[j - 1][2] - it iterates over the whole series. I have edited slightly to hopefully clarify this. \$\endgroup\$
    – jonrsharpe
    Commented May 22, 2014 at 14:40
  • \$\begingroup\$ Oh ok, I get what you meant now. It probably would be more clear as 'the difference of each pair of successive years' or something like that :P \$\endgroup\$ Commented May 22, 2014 at 14:56
  • \$\begingroup\$ @DarinDouglass edited - is that clearer? \$\endgroup\$
    – jonrsharpe
    Commented May 22, 2014 at 14:59
  • \$\begingroup\$ Yeah that looks fine :D \$\endgroup\$ Commented May 22, 2014 at 15:06

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.