1
\$\begingroup\$

What is one way to change it into a program that makes use of OOP concepts? I have always programmed this way and I feel that I would somehow need to improve on it. By the way, I have deliberately avoided pandas and scikit learn.

import csv
from collections import defaultdict
from random import random,randint
import matplotlib.pyplot as plt
import numpy as np
''' This code reads a file with stock price data and does exponential smoothing with the purpose of comparing it with regression on the same column of data. Finally original data and smoothed data are plotted together as well as the regression line along with the scatter plot of the original data. The code allows for repeated input of the smoothing parameter alpha until the user is satisfied with the result.'''
# ------------ Let us check for existence of the file path -------------------------
check_filepath=True
while check_filepath:
    filepath=input('enter the path of the csv file with the stock price data     WITHOUT any  quotations and press Enter: ')
# Example of a file path :  /Users/abigaletessema/Desktop/LYG.csv

# the following try -except-else block catches the error in file path and allows for repeated attempt 
    try:
        f=csv.reader(filepath)
        print("Here is the filepath:"+ filepath) 
    except FileNotFoundError:
        print("The filepath doesn't exist or there are some missing directories , please check and try again")
    else:
        print("Great! the file path is correct. Now, you can carry on!")
        break
# ------------ Let us Open our data file -------------------------
columns = defaultdict(list) # each value in each column is appended to a list

file_handle=open('{}'.format(filepath),'r')
reader=csv.reader(file_handle,delimiter=",")
next(reader,None) # Skips the header of the file which is Time_Index and Adj_Close_Price

for row in reader :
    for (i,v) in enumerate(row):
        columns[i].append(v)

columns[0] = list(map(int, columns[0])) # This list corresponds to the time index
columns[1] = list(map(float, columns[1]))  # This list corresponds to the Adjusted closing price
t,p=columns[0],columns[1] # assigning the above values to a more intuitive name t for time and p for price
print("Time index is {}".format(t))
print("Adjusted closing prices are {} ".format(p))
file_handle.close()

#------------------ Let us do the exponential smoothing part now ----------------
def exponential_smoothing(original_series, alpha):
    smoothed_price = [float(original_series[0])] # first value is same as series
    maximum=len(original_series)
    for n in range(0,maximum-1):
        # The next line populates the smoothed_price list with smoothed values using the stated relationship
        smoothed_price.append(alpha * original_series[n] + (1 - alpha) * smoothed_price[n])

    return smoothed_price

original_series=p



#-------- model parameter selection -----------
def find_the_right_alpha():

    while True:              # keep looping until `break` statement is reached
        alpha_value=float(input("Enter a number between 0 exclusive and 1 inclusive: "))
        try:                 # get ready to catch exceptions inside here
            if 0.0 < alpha_value <= 1.0:

                y=exponential_smoothing(p,alpha_value) # y is another name for smoothed_price
                y = [ round(elem, 2) for elem in y ] # rounding off the y values to 2 significant digits
                print("The smoothed values of the price are {}".format(y))
                        #------------ Plotting time --------------

                plt.plot(t,y,label='Smoothed data',c='r')
                plt.xlabel('Time Index')
                plt.ylabel('Prices')
                plt.plot(t,original_series,label='Original data',c='b')

                plt.grid(True)
                plt.legend()
                plt.show()
                response=input("Do you like the smoothed data ? Enter Y for yes: ")
                # --------checking if the user is happy with the selection of alpha -----------
                if (response =='Y' or response=='y'):
                    print("Great! You seem to be satisfied with alpha value of {}".format(alpha_value))
                    break
                else:
                # This block would give the chosen alpha value as it wouldn't be 
                #executed after the final decision of choice is made
                    alpha_value=find_the_right_alpha() 
                    break
            else:
                print(" Let us try again!")
        except ValueError:      # <-- exception. handle it. loops because of while True
            print("Not a valid alpha value, let's try that again")
    return alpha_value

#---------- End of find_the_right_alpha function-----------------


alpha_value=find_the_right_alpha() # function call to determine alpha

t_predict=len(t)-1
def smoothing_predict(t_predict):
    y=exponential_smoothing(p,alpha_value)
    smooth_predicted=round((alpha_value*p[t_predict]+ (1-alpha_value)*y[t_predict]),3)
    display_value=print(" The smoothing predicted value for time {} is {}".format(t_predict+2,smooth_predicted))
    return display_value

smoothing_predict(t_predict)

# -------------- Regression time--------------


def calc_mean(t):
    total_sum=0
    for i in range(0,len(t)):
        total_sum+=t[i]
        result=total_sum/len(t)
    return result

def calc_coefficients(t,p):

    sum_xy_deviation=0
    sum_xsquared_deviation=0


    for i in range(0,len(t)):
        sum_xy_deviation+=(t[i]-calc_mean(t))*(p[i]-calc_mean(p))

        sum_xsquared_deviation+=pow(t[i]-calc_mean(t),2)

    beta=float(sum_xy_deviation/sum_xsquared_deviation)
    alpha=calc_mean(p)-beta*calc_mean(t)
    coefficients={'alpha':round(alpha,3), 'beta':round(beta,3)}
    return coefficients

def graph_regression_line(regression_formula,t):
    t=np.array(t)
    phat=regression_formula(t)
    plt.title('Regression Line and original data scatter plot')
    plt.scatter(t, p,c='b')
    plt.plot(t,phat)
    plt.show()

def regression_formula(t):
    coefficients=calc_coefficients(t,p)
    return coefficients['alpha']+t*coefficients['beta']
def calc_r_square(t,p):
    SSR,SST=0,0 # SSR is regression sum of squares. SST is total sum of squares
    p_hat=[]
    mean_price=calc_mean(p)
    coefficients=calc_coefficients(t,p)
    for i in range(0,len(t)):
        p_hat.append(coefficients['alpha']+coefficients['beta']*t[i])
        SSR+=pow(p_hat[i]-mean_price,2)
        SST+=pow(p[i]-mean_price,2)
    r_squared=round(SSR/SST,3)
    display_value=print(" The R-Squared value  is {}".format(r_squared))
    return display_value

t_predict=len(t)-1
def regression_predict(t_predict):
    coefficients=calc_coefficients(t,p)
    predicted=round((coefficients['alpha']+t_predict*coefficients['beta']),3)
    display_value= print(" The regression predicted value for time {} is {}".format(t_predict+2,predicted))
    return display_value
#-----------------------Displaying the output of our regression analysis --------------
graph_regression_line(regression_formula,t)
print("\n")
calc_r_square(t,p)
print("\n")
regression_predict(t_predict)
print("----------------")
print("The actual predicted value for time 9 is 3.32")
print("----------------")
\$\endgroup\$
  • 1
    \$\begingroup\$ Hey, could you try pointing out at some ideas of your own regarding code organisation? The material is huge and without docstring is hard to tell who is doing what \$\endgroup\$ – A. Romeu Feb 21 '18 at 13:53
  • \$\begingroup\$ @AndréParamés Comments are for seeking clarification to the question, and may be deleted. Please put all suggestions for improvements in answers, even if your answer is brief. \$\endgroup\$ – 200_success Feb 21 '18 at 14:47
  • \$\begingroup\$ Your code doesn't compile for me. I think you made some errors in spacing when you pasted it into CodeReview. Please copy and re-paste in, then select everything and click on the '{}' icon that formats it as code, rather than trying to properly indent everything yourself. \$\endgroup\$ – Snowbody Feb 21 '18 at 14:50
  • \$\begingroup\$ Also, you need to explain at the top of your file what the code is actually supposed to do, and a brief idea of how it does it. Your post title is a bit cryptic; the phrase "a stock data" is completely incomprehensible. \$\endgroup\$ – Snowbody Feb 21 '18 at 14:51
  • \$\begingroup\$ @ A.Romeu the only idea I have is put the anything related with smoothing in one class and anything related with regression in another. I cannot think of a way to deal with what is common for both of them like the access to the file/data. Thanks for the question. \$\endgroup\$ – Benyam Feb 21 '18 at 15:03
1
\$\begingroup\$

Some programs benefit from being written in an OOP style, some don't. The bigger idea is whether you follow good programming practices. And I see a lot of things that could be improved with your code.

  1. Line lengths. You need to keep your line length below 79 characters per line, and below 72 if you can manage it. You don't want to have to scroll to the right to view the code -- having to do so makes viewing the code later a chore. And you need to write code so that someone reviewing it later who doesn't know everything about it (who may be you -- people forget things) will be able to understand it. It looks like some of the long lines may just be due to you pasting it in incorrectly into CodeReview, but others are definitely too long.
  2. Code organization. There should not be any code at module level other than a "main guard" -- a function that checks if the code is being run interactively and if so calls the real main function that does the work. This allows you to use the functions in the source code .py file as a library.
  3. Use functions. They're part of the language, so use them. Each function should have one responsibility, and all parts of the actual logic of the code should be in functions.
  4. Separate input/output from program logic. You never know if you might have to perform the same types of calculations where the input comes from a file, or a website, or whatever, or where the output has to go to a file. So, write one function that gets the input (looping back if bad input), call another function to do the calculation, and call another for the output.
import csv
from collections import defaultdict
from random import random,randint
import matplotlib.pyplot as plt
import numpy as np
''' This code reads a file with stock price data and does exponential
smoothing with the purpose of comparing it with regression
on the same column of data. Finally original data and smoothed data
are plotted together as well as the regression line along with the
scatter plot of the original data. The code allows for repeated input
of the smoothing parameter alpha until the user is satisfied with the
result.'''
# ------------ Let us check for existence of the file path -------------------------

If there's a comment like this, that's a good signal that the following code should be in its own function, with the comment as a docstring. However, this comment seems to assume that it's about to check some existing string as to whether it's an existent file path, while the code below actually prompts for a string first: the comment is incomplete. So there should be (at least) two functions: one with a parameter which checks the parameter for validity, and another that takes the input and loops if necessary.

check_filepath=True
while check_filepath:
    filepath=input('enter the path of the csv file with the stock price data
    WITHOUT any  quotations and press Enter: ')
# Example of a file path :  /Users/abigaletessema/Desktop/LYG.csv

What's the purpose of the above comment? Do you really expect people reading this code to not understand what a file path is? But if they don't, giving then an example is not really going to help them.

# the following try -except-else block catches the error in file path and allows for repeated attempt 

The above comment is good, because it explains why the code is doing what it's doing.

    try:
        f=csv.reader(filepath)

What happens to f? It seems to never be used again. The code you never closes it, which wastes resources until the process terminates. The call to csv.reader() is wasted with accompanying cost in memory and cpu time. For the right way to check for the existence of a file/validity of a filepath that you're about to open, please see this question.

        print("Here is the filepath:"+ filepath) 

What's the point of this line? It only gets executed if the previous line didn't throw an exception, and it provides no useful information. It's certainly not going to throw an exception, so why is it in the try block? If it's debugging code, could throw it into a logger. Also, you should be using formatted output rather than string concatenation.

    except FileNotFoundError:

Good job making sure to only catch the exception you care about.

        print("The filepath doesn't exist or there are some missing directories , please check and try again")
    else:
        print("Great! the file path is correct. Now, you can carry on!")
        break

Why do you have an else block in this try:/except ...:? Do you know the purpose of an else: block and when it is executed? This code probably belongs outside the try:/except: block.

# ------------ Let us Open our data file -------------------------

Again, this is the place for a new function and a docstring.

columns = defaultdict(list) # each value in each column is appended to a list

This doesn't explain why you're using a defaultdict, which was my first question when I read over the code. It's definitely not the data structure I'd use for this. Each list in the columns variable is going to be the same size, right? I think a better data structure would be a regular list of tuples or objects.

file_handle=open('{}'.format(filepath),'r')

Why are you calling .format() here? What's wrong with just open(filepath,'r')? Also you should really consider using Python's with statement here, to make sure the file is properly closed.

reader=csv.reader(file_handle,delimiter=",")

Why pass the default argument here? Also this is another place for a with

next(reader,None) # Skips the header of the file which is Time_Index and Adj_Close_Price

Good use of the comment!

for row in reader :
    for (i,v) in enumerate(row):
        columns[i].append(v)

Do you know how many columns there will be? Your comment indicates there will always be 2, but the code is written to allow an arbitrary number of columns. Make sure the code matches the comments.

Again, if you used a different data structure, this code would be a lot simpler.

columns[0] = list(map(int, columns[0])) # This list corresponds to the time index
columns[1] = list(map(float, columns[1]))  # This list corresponds to the Adjusted closing price

Why not do this processing as you read the file, instead of making a second pass over it and creating a second temporary list? Why are you sticking things in a dict-of-lists anyway, with a name the the code itself admits is confusing, when you could be sticking them in variables (or object members) that had sensible names?

t,p=columns[0],columns[1] # assigning the above values to a more intuitive name t for time and p for price

Hate to break it to you, but t and p are not "intuitive" variable names. They're cryptic, single character names which convey almost no information. If a reader of this code misses the comment, there's no clue what these mean/contain. The name of a variable should indicate what's inside it and/or what it's used for. Disk space and RAM are cheap (we're not in 1980) so you don't win any prizes for writing the shortest possible variable names. Think of the long-term users of your code. 5 years from now, you may not think it's so intuitive that t means time and p means price.

print("Time index is {}".format(t))
print("Adjusted closing prices are {} ".format(p))

What's the point of these lines? Are they just for debugging? When you're satisfied with the code, removing them will be a hassle. Look into the logging module.

file_handle.close()

#------------------ Let us do the exponential smoothing part now ----------------
def exponential_smoothing(original_series, alpha):

The comment is bad-- it just repeats what the code already says. Comments should explain why the code is there, not just restate what it is doing. Please write a proper docstring for this function -- explain what exponential smoothing is, and what the parameters mean, and what kind of output will be produced.

    smoothed_price = [float(original_series[0])] # first value is same as series

Didn't you already convert the datafile to float? What kind of inputs does this function expect anyway? If its purpose is to perform some kind of smoothing, it shouldn't be doing data type conversions!

    maximum=len(original_series)
    for n in range(0,maximum-1):
        # The next line populates the smoothed_price list with smoothed values using the stated relationship
        smoothed_price.append(alpha * original_series[n] + (1 - alpha) * smoothed_price[n])

This is one of the classic mistakes Python programmers make. I don't know if they're thinking of another language they know, or if they're taught this way, but it reflects a lack of experience with Python and lack of understanding of how to use the language features to make programming easier and less error-prone.

Python's list are first-class language features and should be manipulated as such. You almost never have to manually generate list indexes and iterate over the list. Instead you should use list comprehensions which will get this whole function done in one easy-to-read line.

    return smoothed_price

original_series=p



#-------- model parameter selection -----------
def find_the_right_alpha():

All functions should have docstrings. Comments are not a substitute. BTW this particular comment is completely inscrutable.

    while True:              # keep looping until `break` statement is reached
        alpha_value=float(input("Enter a number between 0 exclusive and 1 inclusive: "))

Oops, you forgot to check for bad input. A non-float entered here crashes the program.

        try:                 # get ready to catch exceptions inside here
            if 0.0 < alpha_value <= 1.0:

                y=exponential_smoothing(p,alpha_value) # y is another name for smoothed_price
                y = [ round(elem, 2) for elem in y ] # rounding off the y values to 2 significant digits
                print("The smoothed values of the price are {}".format(y))
                        #------------ Plotting time --------------

                plt.plot(t,y,label='Smoothed data',c='r')
                plt.xlabel('Time Index')
                plt.ylabel('Prices')
                plt.plot(t,original_series,label='Original data',c='b')

                plt.grid(True)
                plt.legend()
                plt.show()
                response=input("Do you like the smoothed data ? Enter Y for yes: ")
                # --------checking if the user is happy with the selection of alpha -----------
                if (response =='Y' or response=='y'):
                    print("Great! You seem to be satisfied with alpha value of {}".format(alpha_value))
                    break
                else:
                # This block would give the chosen alpha value as it wouldn't be 
                #executed after the final decision of choice is made
                    alpha_value=find_the_right_alpha() 

You're using recursion here, probably because it's what they taught you. Recursion is generally taught to make students think, not because it's an essential programming technique. In this case, recursion is a bad idea. Try to rearrange the function so it doesn't call itself.

                    break
            else:
                print(" Let us try again!")
        except ValueError:      # <-- exception. handle it. loops because of while True
            print("Not a valid alpha value, let's try that again")
    return alpha_value

#---------- End of find_the_right_alpha function-----------------

This comment will become unnecessary when you rearrange the code into multiple functions.

alpha_value=find_the_right_alpha() # function call to determine alpha

t_predict=len(t)-1
def smoothing_predict(t_predict):
    y=exponential_smoothing(p,alpha_value)
    smooth_predicted=round((alpha_value*p[t_predict]+ (1-alpha_value)*y[t_predict]),3)
    display_value=print(" The smoothing predicted value for time {} is {}".format(t_predict+2,smooth_predicted))
    return display_value

smoothing_predict(t_predict)

# -------------- Regression time--------------


def calc_mean(t):
    total_sum=0
    for i in range(0,len(t)):
        total_sum+=t[i]
        result=total_sum/len(t)
    return result

In addition to the for i in range anti-pattern, the above function is unnecessarily inefficient. See if you can figure out why.

Also, I know you are avoiding specialized calculation libraries, but you really ought to be using the Python built-in sum() here.

def calc_coefficients(t,p):

    sum_xy_deviation=0
    sum_xsquared_deviation=0


    for i in range(0,len(t)):

Same thing. This time you'll want to use the zip() builtin to operate on two lists of the same length.

        sum_xy_deviation+=(t[i]-calc_mean(t))*(p[i]-calc_mean(p))

        sum_xsquared_deviation+=pow(t[i]-calc_mean(t),2)

Why the repeated calls to calc_mean(t)? It's not going to change.

    beta=float(sum_xy_deviation/sum_xsquared_deviation)
    alpha=calc_mean(p)-beta*calc_mean(t)
    coefficients={'alpha':round(alpha,3), 'beta':round(beta,3)}
    return coefficients

def graph_regression_line(regression_formula,t):
    t=np.array(t)
    phat=regression_formula(t)
    plt.title('Regression Line and original data scatter plot')
    plt.scatter(t, p,c='b')
    plt.plot(t,phat)
    plt.show()

This is good, you have all your plotting code in one function.

def regression_formula(t):
    coefficients=calc_coefficients(t,p)
    return coefficients['alpha']+t*coefficients['beta']
def calc_r_square(t,p):
    SSR,SST=0,0 # SSR is regression sum of squares. SST is total sum of squares

It really doesn't hurt to have longer variable names. It really doesn't take long to type them out, and most editors will autocomplete them for you. The comment should explain what the variables are used for, not expand the acronym.

    p_hat=[]
    mean_price=calc_mean(p)
    coefficients=calc_coefficients(t,p)
    for i in range(0,len(t)):
        p_hat.append(coefficients['alpha']+coefficients['beta']*t[i])
        SSR+=pow(p_hat[i]-mean_price,2)
        SST+=pow(p[i]-mean_price,2)
    r_squared=round(SSR/SST,3)
    display_value=print(" The R-Squared value  is {}".format(r_squared))

That's an awfully strange way to use print().

    return display_value

t_predict=len(t)-1
def regression_predict(t_predict):
    coefficients=calc_coefficients(t,p)
    predicted=round((coefficients['alpha']+t_predict*coefficients['beta']),3)
    display_value= print(" The regression predicted value for time {} is {}".format(t_predict+2,predicted))
    return display_value
#-----------------------Displaying the output of our regression analysis --------------
graph_regression_line(regression_formula,t)
print("\n")
calc_r_square(t,p)
print("\n")
regression_predict(t_predict)
print("----------------")
print("The actual predicted value for time 9 is 3.32")
print("----------------")
\$\endgroup\$
  • \$\begingroup\$ I cannot thank you enough. I will have to print out your comments and go through each one of them until the problems are all addressed. Thanks so much. \$\endgroup\$ – Benyam Mar 7 '18 at 16:09
  • \$\begingroup\$ You're welcome @Benyam! Once the code is in better overall shape, we can discuss the areas in which using objects would improve things. \$\endgroup\$ – Snowbody Mar 7 '18 at 20:27
  • \$\begingroup\$ I will surely get back to you \$\endgroup\$ – Benyam Mar 12 '18 at 1:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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