My submission #3
Conversation
There was a problem hiding this comment.
Hi Brandon,
This was a good attempt. I can see that you made a lot of effort and did your homework, however you made some mistakes in your implementation and some of your design choices highlight inexperience.
That is alright, considering that most of our researchers already have a masters degree. If you keep this spirit up, I think you will do well in your career.
We will setup an interview.
- Good writeup!
- Reviewing previous candidates' reviews paid off big time!
- Nice code style
- Good use of hidden and public methods
- Wrong implementation
- Good inline comments and docstrings
- Added req file
- Good design ideas
- Some bad design choices
- Would benefit by adding a linter.
- Cool that you added web scraping
- Would have been nice to see at least 1 unit test.
brandon_tan/requirements.txt
Outdated
| @@ -0,0 +1,52 @@ | |||
| appnope==0.1.2 | |||
There was a problem hiding this comment.
Good that you added a requirements file but you have added way too many dependencies here. Are you really using all of these libraries in your code?
There was a problem hiding this comment.
Hi Jacques.
I used a virtual environment and installed the relevant packages. Did not remove some as I was not sure if they had dependencies. Will take note in the future!
brandon_tan/partner_selector.py
Outdated
| @@ -0,0 +1,483 @@ | |||
| import pandas as pd | |||
| import numpy as np | |||
|
|
|||
There was a problem hiding this comment.
Please tidy up your imports and use the correct import order.
|
|
||
| class PartnerSelector: | ||
| """ | ||
| Implementation of the Partner Selection Framework. |
There was a problem hiding this comment.
This docstring might be better for a module docstring. Your class docstring should relate to what that class does. I would move the H&T skillset challenge to the module docstring.
Nice that you added a link to the paper, but you should also have added the name of the paper.
| https://www.econstor.eu/bitstream/10419/147450/1/870932616.pdf | ||
| """ | ||
|
|
||
| def __init__(self, price_data: pd.DataFrame, target_ticker: str, no_of_partners: int = 3, top_n_corr: int = 50): |
There was a problem hiding this comment.
Good that you added hard typing.
| :param price_data: Consumes a price dataframe provided by the user. Index by datetime and columns are tickers. Implicitly, the columns will define the universe that the partners will be found in. | ||
| :type price_data: pd.DataFrame | ||
| :param target_ticker: The target ticker that the user would like to create the grouping around. | ||
| :type price_data: str |
There was a problem hiding this comment.
I have not seen this style of docstring before. Are you sure this will render in Sphinx? `:type:
There was a problem hiding this comment.
I used the VSC extension by Nils Werner - "Python Docstring Generator" and set the settings to Sphinx. Let me check on this again.
|
|
||
| def get_index_ticker(index: str): | ||
| """ | ||
| Function to scrape existing index members from CNN website. |
|
|
||
| for i in tqdm(range(len(self.all_possible_partners))): | ||
|
|
||
| # Converting to list as itertools combination generates an itertools object. |
There was a problem hiding this comment.
I'm seeing a lot of duplicate code here. As noted in your write-up, this should be more modular.
|
|
||
| def plot_selected_partners(self): | ||
| """ | ||
| Plots the historical log prices of the best partners that was identified |
There was a problem hiding this comment.
In the write up you mention that you are not sure what some good visualizations would be. It would be good to show visually, how the various techniques extract different properties from the data. Something to highlight the differences, which technique is better, something along those lines.
| @@ -0,0 +1,436 @@ | |||
| { | |||
| "metadata": { | |||
There was a problem hiding this comment.
The notebook is good but would benefit from a body section that describes the various techniques.
| { | ||
| "metadata": { | ||
| "language_info": { | ||
| "codemirror_mode": { |
Hi Team,
Here is my submission. Thank you!