MN Submission #4
Conversation
There was a problem hiding this comment.
Hi Minh,
This implementation is ok. It looks like you didn't put in the extra effort that we were hoping for. I can see that you have programming experience.
- Good to see hidden and public methods
- Nice use of inline comments
- Matches our style of docstrings
- Questionable design choices. I think it's wrong but maybe you can explain why you did it.
- Using class inheritance may have been a better solution.
- Incorrect implementation of the extended method
- Notebook is a bit sparse. I would have liked to see more detailed descriptions of the various techniques.
| @@ -0,0 +1,238 @@ | |||
| import pandas as pd | |||
| import pandas as pd | ||
| import numpy as np | ||
|
|
||
| from itertools import combinations |
|
|
||
|
|
||
| class PartnerSelector(): | ||
| """An implementation of the Partner Selection methods as described in |
There was a problem hiding this comment.
Nice docstring. I like the attributes section but wonder if it will work with Sphinx.
| self.data = data_prices | ||
| data_returns = data_prices.pct_change().iloc[1:] | ||
| # Transpose to set index as ticker symbols for convenience | ||
| self.data_returns = data_returns.transpose() |
There was a problem hiding this comment.
I am a little unsure about this step. I think it will affect your corr matrix.
|
|
||
| # Internal data structure, keys are target tickers and values are DataFrames | ||
| # containing potential combinations along with various association metrics | ||
| self._partner_data = {ticker:self._get_combination_df(ticker, self._threshold) for ticker in tqdm(self.data_corr.index, desc="Calculating preselections")} |
| # containing potential combinations along with various association metrics | ||
| self._partner_data = {ticker:self._get_combination_df(ticker, self._threshold) for ticker in tqdm(self.data_corr.index, desc="Calculating preselections")} | ||
|
|
||
| def get_partners(self, method=Method.TRADITIONAL, targets=None): |
There was a problem hiding this comment.
Hmm I saw you introduce this technique but I am not convinced that this is the best way to do it.
Maybe you can please elaborate as to why you did it this way?
There was a problem hiding this comment.
This is quite an awkward way to call the traditional method. selector.get_partners(method=PartnerSelector.Method.TRADITIONAL, targets=target_tickers).
The user needs to know your internal class structure to know that they need to call PartnerSelector.Method.TRADITIONAL inside this function.
That seems like a bad idea, what am I missing?
There was a problem hiding this comment.
This seems like an artifact from C# or maybe Java.
| """Get the partner combinations. The first ticker in each combination is considerd the 'target' ticker. | ||
|
|
||
| :param method: (str): the method by which to determine partner associations, see `PartnerSelector.Method` | ||
| :param targets: (list): if supplied, only retrieve partners for tickers in this list |
There was a problem hiding this comment.
Good that you followed our style of docstrings.
| self.dimensions = dimensions | ||
| self._threshold = threshold | ||
|
|
||
| # Internal data structure, keys are target tickers and values are DataFrames |
| """ | ||
| target_data = self._filter_targets(targets) | ||
|
|
||
| if method == PartnerSelector.Method.TRADITIONAL: |
There was a problem hiding this comment.
Would making use class inheritance not be a better solution to this?
|
|
||
| for df in tqdm(target_data.values(), desc="Calculating extended Spearman associations"): | ||
| results = np.array([calc_spearman_ext(row.tickers_idx) for row in df.itertuples()]) | ||
| df[PartnerSelector.Method.EXTENDED] = np.average(results, axis=1) |
Here's my submission, thanks!