current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

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
share|improve this question
add comment

2 Answers

up vote 3 down vote accepted

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
share|improve this answer
add comment
up vote 2 down vote

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]
            ...
share|improve this answer
    
He doesn't want a static second window of size two. Its of size period. –  BeetDemGuise May 22 at 14:36
    
@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. –  jonrsharpe May 22 at 14:40
    
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 –  BeetDemGuise May 22 at 14:56
    
@DarinDouglass edited - is that clearer? –  jonrsharpe May 22 at 14:59
    
Yeah that looks fine :D –  BeetDemGuise May 22 at 15:06
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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