Add files via upload

[ghost]

Hi Jacques,

I have finished the first version of my implementation of the pair selection section in Statistical Arbitrage with Vine Copulas.

[Jackal08]

Hi Jamie,

I think perhaps python is not the language you usually code in? The code style, design choices, and notebook could use quite a bit more work. I hope that my review can help you to improve going forward.

• There is way too much duplicate code
• Not following Python conventions
• Didn't use a linter
• Didn't use the class structure correctly
• All methods are public
• Param names need to be improved
• Missing docstrings
• Way too many white spaces
• Inconsistent style. Sometimes CamelCase others snake_case.
• The user needs to do both the basic processing and the desired approach. Why?
• Visualisations don't really communicate the value of the Partner Selection.
• Notebook is missing body text.

[ghost]

Hi Jacques, Thank you for your feedback. To be honest, I have no experience developing a package for users before and I did not know there are so many details behind it. Still, the revised version of the implementation of the parter section was completed and uploaded to Github. Thank you for this learning opportunity. Sincerely, Jui Lin(Jamie) Keng MS - Graduate Student Computational Finance and Risk Management University of Washington (206) 468-5457 [email protected] Jacques Joubert <[email protected]> 於 2021年2月18日 週四 上午10:18寫道：

…

***@***.**** requested changes on this pull request. Hi Jamie, I think perhaps python is not the language you usually code in? The code style, design choices, and notebook could use quite a bit more work. I hope that my review can help you to improve going forward. - There is way too much duplicate code - Not following Python conventions - Didn't use a linter - Didn't use the class structure correctly - All methods are public - Param names need to be improved - Missing docstrings - Way too many white spaces - Inconsistent style. Sometimes CamelCase others snake_case. - The user needs to do both the basic processing and the desired approach. Why? - Visualisations don't really communicate the value of the Partner Selection. - Notebook is missing body text. ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > @@ -0,0 +1,300 @@ +import pandas as pd +import numpy as np + +""" + +Class approach_choice offers four different approaches for users to choose from in the partner selection phase. + Why the big whitespace? ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > @@ -0,0 +1,300 @@ +import pandas as pd +import numpy as np + +""" + +Class approach_choice offers four different approaches for users to choose from in the partner selection phase. + + +""" + +class approach_choice: + Missing class docstring. Classes should not be snake case in python. ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > + + +""" + +class approach_choice: + + def __init__(self, return_df): + """ + Setting up class attribute. + """ + self.return_df = return_df + + + def euclidean_distance(self, array): + """ + This docstring doesn't match our convention. ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > + Calculate the Euclidean distance between a point representing ranks of stocks and + the diagonal line starting from point (0,0,0,0) to point (1,1,1,1) in four-dimensional space. + + Parameters + ---------- + array : numpy array + + Returns + ------- + distance : float + The calculated sum of distance from each points to the four-dimensional diagonal line. + + """ + + distance = np.linalg.norm( array - ***@***.***([1,1,1,1])/4 * np.array([1,1,1,1]) ) + So much white space... ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > + +Class approach_choice offers four different approaches for users to choose from in the partner selection phase. + + +""" + +class approach_choice: + + def __init__(self, return_df): + """ + Setting up class attribute. + """ + self.return_df = return_df + + + def euclidean_distance(self, array): Bad param name.array ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > + +Class approach_choice offers four different approaches for users to choose from in the partner selection phase. + + +""" + +class approach_choice: + + def __init__(self, return_df): + """ + Setting up class attribute. + """ + self.return_df = return_df + + + def euclidean_distance(self, array): This is a static method. ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > + def __init__(self, return_df): + """ + Setting up class attribute. + """ + self.return_df = return_df + + + def euclidean_distance(self, array): + """ + + Calculate the Euclidean distance between a point representing ranks of stocks and + the diagonal line starting from point (0,0,0,0) to point (1,1,1,1) in four-dimensional space. + + Parameters + ---------- + array : numpy array Bad description of param. ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > + """ + + Calculate the Euclidean distance between a point representing ranks of stocks and + the diagonal line starting from point (0,0,0,0) to point (1,1,1,1) in four-dimensional space. + + Parameters + ---------- + array : numpy array + + Returns + ------- + distance : float + The calculated sum of distance from each points to the four-dimensional diagonal line. + + """ + Did you run this code through a linter? ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > + + Returns + ------- + distance : float + The calculated sum of distance from each points to the four-dimensional diagonal line. + + """ + + distance = np.linalg.norm( array - ***@***.***([1,1,1,1])/4 * np.array([1,1,1,1]) ) + + + return distance + + + + def traditional(self, final_pair_dict, df_corr): Should this be a public or private method? ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > + + approach : string + The name of the apporach a user chooses to conduct partner selection. + (valid inputs: "traditional", "extended", "geometric", "extremal") + + Returns + ------- + result_pair_dict : dictionary + A dictionary with one final quadruple for each target stock. + + keys : target stocks + values : The one and only quadruple selected by any approach's mechanism for each target stock. + + """ + + result_pair_dict = {} A better way is to make use of class hierarchy. You should rather have made classes for each technique and they can inherit from a base class. ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > + c = self.return_df.iloc[:,indexes[2]].rank()/ length + d = self.return_df.iloc[:,indexes[3]].rank()/ length + + # A created list to store distance values + ### Implementation of Proposition 3 on page 17 + density_derivative= [] + for j in range(0, len(self.return_df)): + + ans= 1 - 2*(a[j]+b[j]+c[j]+d[j]) + 4*(a[j]*b[j]+a[j]*c[j]+a[j]*d[j]+b[j]*c[j]+b[j]*d[j]+c[j]*d[j])- \ + 8*(a[j]*b[j]*c[j] + a[j]*b[j]*d[j] + a[j]*c[j]*d[j] + b[j]*c[j]*d[j]) + \ + 16*(a[j]*b[j]*c[j]*d[j]) + + density_derivative.append(ans) + + + # Mean of the values of density derivative I like your use of inline comments. It improves readability. ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > + values : chi square statistic for each quadruple candidate. + + """ + + pair_derivative_val_dict = {} + + for target_stock in final_pair_dict: + pair_derivative_val_dict[target_stock] = {} + + for pair_num in range(0,len(final_pair_dict[target_stock])): + + # Indexes of target stock + 3 partner stocks on the dataset, sp500 + indexes = list(list(final_pair_dict[target_stock])[pair_num]) + + + # Normalized ranks of each stocks stocks? ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > + The spearman correlation matrix computed by a built-in pandas method. + + + Returns + ------- + pair_rho_val_dict: dictionary + A dictionary containing multivariate version of Spearman's rho for each quadruple. + (The algorithm is based on the empirical expression for multivariate Spearman's correlation.) + + keys : target stocks + values : multivariate Spearman's rho for each quadruple candidate. + + """ + pair_rho_val_dict = {} + + for target_stock in final_pair_dict: You are copy-pasting this pattern in each of your functions. Why didn't you make the code more modular? ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > + A dictionary containing sum of Euclidean distance between points representing ranks of stocks and + the diagonal line starting from point (0,0,0,0) to point (1,1,1,1) in four-dimensional space. + + keys : target stocks + values : Euclidean distance summation of each quadruple candidates. + + """ + + pair_dist_val_dict = {} + + for target_stock in final_pair_dict: + pair_dist_val_dict[target_stock] = {} + + for pair_num in range(0,len(final_pair_dict[target_stock])): + + # Indexes of target stock + 3 partner stocks on the dataset, sp500 There is so much duplicate code here. ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > + +Class approach_choice offers four different approaches for users to choose from in the partner selection phase. + + +""" + +class approach_choice: + + def __init__(self, return_df): + """ + Setting up class attribute. + """ + self.return_df = return_df + + + def euclidean_distance(self, array): This method should be private and not public. ------------------------------ In Jamie_Keng/basic_processing.py <#1 (comment)> : > @@ -0,0 +1,105 @@ +import heapq +import pandas as pd +import numpy as np +from itertools import permutations + +""" +Class basic_processing provides a function to filter out potential highly correlated stocks +for each stock in the dataset. Users have the option to set the number of potential partner stocks they want. + +""" + +class basic_processing: + + def __init__(self): Why an empty constructor? ------------------------------ In Jamie_Keng/basic_processing.py <#1 (comment)> : > @@ -0,0 +1,105 @@ +import heapq +import pandas as pd +import numpy as np +from itertools import permutations + +""" +Class basic_processing provides a function to filter out potential highly correlated stocks +for each stock in the dataset. Users have the option to set the number of potential partner stocks they want. + +""" + +class basic_processing: This is a static class? ------------------------------ In Jamie_Keng/basic_processing.py <#1 (comment)> : > + +""" + +class basic_processing: + + def __init__(self): + pass + + + def partner_selection(self, ret_df, partner_num): + """ + + Parameters + ---------- + ret_df : pandas dataframe + The return dataset generated in the preliminary step. Please use better descriptions. I don't know what the preliminary step is. ------------------------------ In Jamie_Keng/basic_processing.py <#1 (comment)> : > + +""" +Class basic_processing provides a function to filter out potential highly correlated stocks +for each stock in the dataset. Users have the option to set the number of potential partner stocks they want. + +""" + +class basic_processing: + + def __init__(self): + pass + + + def partner_selection(self, ret_df, partner_num): + """ + Where is your description? ------------------------------ In Jamie_Keng/basic_processing.py <#1 (comment)> : > + (The larger the number is, the more computaionally expensive it is.) + + Returns + ------- + partner_dict : dictionary + A dictionary containing all possible partner-stock candidates for each target stock. + + + """ + + # https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.corr.html + + + # Obtain pairwise Spearsman's rho in thw dataset. + df_corr = ret_df.corr(method = "spearman") + #df_corr You need to remove commented out code. ------------------------------ In Jamie_Keng/basic_processing.py <#1 (comment)> : > + A dictionary containing all possible partner-stock candidates for each target stock. + + + """ + + # https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.corr.html + + + # Obtain pairwise Spearsman's rho in thw dataset. + df_corr = ret_df.corr(method = "spearman") + #df_corr + + # Build an empty dictionary to store a number of top partners for each target stock. + partner_dict = {target: None for target in range(len(df_corr))} + + #partner_dict Missing space ------------------------------ In Jamie_Keng/basic_processing.py <#1 (comment)> : > + for target_stock in range(len(df_corr)): + lst = [] + for stock in range(len(df_corr)): + lst.append(df_corr.iloc[target_stock][stock]) + # Find the list of the largest 50 stock's indexes + top_partner = heapq.nlargest(partner_num, range(len(lst)), key=lst.__getitem__) + + partner_dict[target_stock]= top_partner + + + return df_corr, partner_dict + + + def potential_quadruples(self, partner_dict): + """ + Parameters Docstring missing ------------------------------ In Jamie_Keng/basic_processing.py <#1 (comment)> : > +import numpy as np +from itertools import permutations + +""" +Class basic_processing provides a function to filter out potential highly correlated stocks +for each stock in the dataset. Users have the option to set the number of potential partner stocks they want. + +""" + +class basic_processing: + + def __init__(self): + pass + + + def partner_selection(self, ret_df, partner_num): These are all supposed to be static methods but you are missing the decorator. Do these need to be in a class structure? ------------------------------ In Jamie_Keng/visualization.py <#1 (comment)> : > @@ -0,0 +1,176 @@ +import matplotlib.pyplot as plt +import seaborn as sns +from collections import Counter +from sklearn.decomposition import PCA Wrong import order ------------------------------ In Jamie_Keng/visualization.py <#1 (comment)> : > + The corresponding index of a target stock. + + + Reference + --------- + http://rasbt.github.io/mlxtend/user_guide/plotting/scatterplotmatrix/ + + """ + + sns.set_theme(style="darkgrid") + + df = self.return_df.iloc[:,list(approach_result[target_stock_index])] + sns.pairplot(df) + + + def CountFrequency(self, my_list): Why is this method in capital letters? ------------------------------ In Jamie_Keng/basic_processing.py <#1 (comment)> : > + +""" +Class basic_processing provides a function to filter out potential highly correlated stocks +for each stock in the dataset. Users have the option to set the number of potential partner stocks they want. + +""" + +class basic_processing: + + def __init__(self): + pass + + + def partner_selection(self, ret_df, partner_num): + """ + ? ------------------------------ In Jamie_Keng/basic_processing.py <#1 (comment)> : > + ret_df : pandas dataframe + The return dataset generated in the preliminary step. + + partner_num : int + The number of potential partner stocks that a user want for a target stock. + (The larger the number is, the more computaionally expensive it is.) + + Returns + ------- + partner_dict : dictionary + A dictionary containing all possible partner-stock candidates for each target stock. + + + """ + + # https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.corr.html Bad indentation ------------------------------ In Jamie_Keng/basic_processing.py <#1 (comment)> : > + (The larger the number is, the more computaionally expensive it is.) + + Returns + ------- + partner_dict : dictionary + A dictionary containing all possible partner-stock candidates for each target stock. + + + """ + + # https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.corr.html + + + # Obtain pairwise Spearsman's rho in thw dataset. + df_corr = ret_df.corr(method = "spearman") + #df_corr Remove code that you commented out. ------------------------------ In Jamie_Keng/basic_processing.py <#1 (comment)> : > + A dictionary containing all possible partner-stock candidates for each target stock. + + + """ + + # https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.corr.html + + + # Obtain pairwise Spearsman's rho in thw dataset. + df_corr = ret_df.corr(method = "spearman") + #df_corr + + # Build an empty dictionary to store a number of top partners for each target stock. + partner_dict = {target: None for target in range(len(df_corr))} + + #partner_dict Space ------------------------------ In Jamie_Keng/basic_processing.py <#1 (comment)> : > + + + # Obtain pairwise Spearsman's rho in thw dataset. + df_corr = ret_df.corr(method = "spearman") + #df_corr + + # Build an empty dictionary to store a number of top partners for each target stock. + partner_dict = {target: None for target in range(len(df_corr))} + + #partner_dict + for target_stock in range(len(df_corr)): + lst = [] + for stock in range(len(df_corr)): + lst.append(df_corr.iloc[target_stock][stock]) + # Find the list of the largest 50 stock's indexes + top_partner = heapq.nlargest(partner_num, range(len(lst)), key=lst.__getitem__) I stopped at this point. ------------------------------ In Jamie_Keng/visualization.py <#1 (comment)> : > + The corresponding index of a target stock. + + + Reference + --------- + http://rasbt.github.io/mlxtend/user_guide/plotting/scatterplotmatrix/ + + """ + + sns.set_theme(style="darkgrid") + + df = self.return_df.iloc[:,list(approach_result[target_stock_index])] + sns.pairplot(df) + + + def CountFrequency(self, my_list): Why CamelCase here? ------------------------------ In Jamie_Keng/visualization.py <#1 (comment)> : > + df = self.return_df.iloc[:,list(approach_result[target_stock_index])] + sns.pairplot(df) + + + def CountFrequency(self, my_list): + + # An empty dictionary + freq = {} + for items in my_list: + freq[items] = my_list.count(items) -1 + + + return freq + + + def Freq_partner_stocks(self, approach_result, stock_ind_dict, num =10): ? ------------------------------ In Jamie_Keng/skillset_challenge.ipynb <#1 (comment)> : > @@ -0,0 +1,595 @@ +{ + "cells": [ + { It's not pairs selection. It's partner selection. ------------------------------ In Jamie_Keng/skillset_challenge.ipynb <#1 (comment)> : > @@ -0,0 +1,595 @@ +{ + "cells": [ + { + "cell_type": "markdown", + "metadata": {}, + "source": [ Why only 89 constituents? ------------------------------ In Jamie_Keng/skillset_challenge.ipynb <#1 (comment)> : > @@ -0,0 +1,595 @@ +{ + "cells": [ + { + "cell_type": "markdown", + "metadata": {}, + "source": [ + "# <center> Statistical Arbitrage with Vine Copulas - Pair Selection </center>\n", + "\n", + "Author: Jui Lin(Jamie) Keng <br>\n", + "Email: ***@***.*** <br>\n", + "LinkedIn : https://www.linkedin.com/in/jui-lin-jamie-keng-3883b6131/" Nice use of markdown. ------------------------------ In Jamie_Keng/skillset_challenge.ipynb <#1 (comment)> : > @@ -0,0 +1,595 @@ +{ + "cells": [ + { + "cell_type": "markdown", + "metadata": {}, + "source": [ + "# <center> Statistical Arbitrage with Vine Copulas - Pair Selection </center>\n", + "\n", + "Author: Jui Lin(Jamie) Keng <br>\n", + "Email: ***@***.*** <br>\n", + "LinkedIn : https://www.linkedin.com/in/jui-lin-jamie-keng-3883b6131/" + ] + }, + { + "cell_type": "markdown", Why does your implementation require the user to perform basic processing? This should be handled internally. Ideally, a user would import a class and then can call as few methods as possible to get to their end goal. ------------------------------ In Jamie_Keng/skillset_challenge.ipynb <#1 (comment)> : > + "cells": [ + { + "cell_type": "markdown", + "metadata": {}, + "source": [ + "# <center> Statistical Arbitrage with Vine Copulas - Pair Selection </center>\n", + "\n", + "Author: Jui Lin(Jamie) Keng <br>\n", + "Email: ***@***.*** <br>\n", + "LinkedIn : https://www.linkedin.com/in/jui-lin-jamie-keng-3883b6131/" + ] + }, + { + "cell_type": "markdown", + "metadata": {}, + "source": [ It would have been good to explain each approach. Your notebook is missing a body section. ------------------------------ In Jamie_Keng/skillset_challenge.ipynb <#1 (comment)> : > + "metadata": {}, + "source": [ + "# <center> Statistical Arbitrage with Vine Copulas - Pair Selection </center>\n", + "\n", + "Author: Jui Lin(Jamie) Keng <br>\n", + "Email: ***@***.*** <br>\n", + "LinkedIn : https://www.linkedin.com/in/jui-lin-jamie-keng-3883b6131/" + ] + }, + { + "cell_type": "markdown", + "metadata": {}, + "source": [ + "# Introduction\n", + "\n", + "This notebook demonstrates four different approaches to the pair selection methods described in section three in [1]. The goal is to select three of the most suitable partner stocks given a target stock to form a quadruple. Based on different measures of dependence, each approach yields similar results, surprisingly. <br>\n", I'm not sure how to interpret your scatter plot? What insights can we derive from this? ------------------------------ In Jamie_Keng/skillset_challenge.ipynb <#1 (comment)> : > + "\n", + "Author: Jui Lin(Jamie) Keng <br>\n", + "Email: ***@***.*** <br>\n", + "LinkedIn : https://www.linkedin.com/in/jui-lin-jamie-keng-3883b6131/" + ] + }, + { + "cell_type": "markdown", + "metadata": {}, + "source": [ + "# Introduction\n", + "\n", + "This notebook demonstrates four different approaches to the pair selection methods described in section three in [1]. The goal is to select three of the most suitable partner stocks given a target stock to form a quadruple. Based on different measures of dependence, each approach yields similar results, surprisingly. <br>\n", + "\n", + "Please note that it is computationally expensive to find $\\binom{50}{3}$ potential partner-stock combinations for every single constituent in S&P500. Therefore, I designed functions in a way that users can choose the number of target stocks out of S&P500, and the number of potential partner-stock combinations for each target stocks. <br>\n", + "\n", Your conclusion is likely affected by your very small universe (81 stocks). ------------------------------ In Jamie_Keng/approach_choice.py <#1 (comment)> : > + + +""" + +class approach_choice: + + def __init__(self, return_df): + """ + Setting up class attribute. + """ + self.return_df = return_df + + + def euclidean_distance(self, array): + """ + Also rm whitespace. ------------------------------ In Jamie_Keng/skillset_challenge.ipynb <#1 (comment)> : > + "metadata": {}, + "source": [ + "# <center> Statistical Arbitrage with Vine Copulas - Pair Selection </center>\n", + "\n", + "Author: Jui Lin(Jamie) Keng <br>\n", + "Email: ***@***.*** <br>\n", + "LinkedIn : https://www.linkedin.com/in/jui-lin-jamie-keng-3883b6131/" + ] + }, + { + "cell_type": "markdown", + "metadata": {}, + "source": [ + "# Introduction\n", + "\n", + "This notebook demonstrates four different approaches to the pair selection methods described in section three in [1]. The goal is to select three of the most suitable partner stocks given a target stock to form a quadruple. Based on different measures of dependence, each approach yields similar results, surprisingly. <br>\n", Same thing with the PCA plot. ------------------------------ In Jamie_Keng/skillset_challenge.ipynb <#1 (comment)> : > + "metadata": {}, + "source": [ + "# <center> Statistical Arbitrage with Vine Copulas - Pair Selection </center>\n", + "\n", + "Author: Jui Lin(Jamie) Keng <br>\n", + "Email: ***@***.*** <br>\n", + "LinkedIn : https://www.linkedin.com/in/jui-lin-jamie-keng-3883b6131/" + ] + }, + { + "cell_type": "markdown", + "metadata": {}, + "source": [ + "# Introduction\n", + "\n", + "This notebook demonstrates four different approaches to the pair selection methods described in section three in [1]. The goal is to select three of the most suitable partner stocks given a target stock to form a quadruple. Based on different measures of dependence, each approach yields similar results, surprisingly. <br>\n", Ok, I see the analysis section. It would have been better to add the analysis under the plot. I'm not sure that your choice of visualization communicates value w.r.t quadruples. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJAON4XIJR52WE7WBDUJ4ELS7VKYBANCNFSM4XVSGEPA> .