Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/attribution toolkit #3156

Merged
merged 25 commits into from
Nov 11, 2022

Conversation

Sammac-dev
Copy link
Contributor

@Sammac-dev Sammac-dev commented Oct 29, 2022

Description

  • Attribution Toolkit, allows users to calculate and visualize both portfolio attribution by sector and benchmark attribution by sector for the S&P500.

To use attribution toolkit run "attrib" from portfolio section after loading in portfolio

image
image
image

Requested by Product Manager Jeroen Bouma

  • Added dependency - yahooquery==2.2.10

How has this been tested?

This feature has been tested locally by running the terminal and checking that the outputs display correctly and as expected. The feature is found within the portfolio menu and is called via "attrib". The feature has been executed with default time period of all (using first date found in the portfolio loaded by the user) via the "attrib" command and also with different time periods selected such as "attrib -p 3m".

Checklist:

Others

  • [X ] I have performed a self-review of my own code.
  • [X ] I have commented my code, particularly in hard-to-understand areas.

@JerBouma JerBouma self-requested a review October 29, 2022 07:21
@JerBouma
Copy link
Contributor

Awesome @Sammac-dev, I'll be deep diving into this also checking if the financial theory behind it is correct. Will get back to you soon 😁

@JerBouma JerBouma added feat L Large T-Shirt size Feature portfolio Portfolio menu labels Oct 29, 2022
@jmaslek
Copy link
Collaborator

jmaslek commented Oct 29, 2022

Thanks @Sammac-dev. This looks awesome!

One question: can you replace the yahoo query with yfinance. Since you can get the sector weightings with yf.Ticker("SPY").info["sectorWeightings"]. With a massive list of dependencies, I am inclined to stick to the package we have that already does this, just to not keep adding to that list.

Granted - wow this is faster
Screen Shot 2022-10-29 at 11 25 32 AM

@Sammac-dev
Copy link
Contributor Author

Sammac-dev commented Oct 30, 2022

Thanks @JerBouma
@jmaslek no problems I can remove the yahooquery dependencies and replace with yahoofinance. Also, as this is our first time contributing to an open-source project I just had a quick question, In order to send the updates to you do I simply commit to my feature/AttributionToolkit branch or do I need to do something else?

@jmaslek
Copy link
Collaborator

jmaslek commented Oct 30, 2022

Yes you can just commit to your branch!

@Sammac-dev
Copy link
Contributor Author

@jmaslek that's been taken care of, I've committed the new code

Copy link
Contributor

@JerBouma JerBouma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Sammac-dev, I made quite some suggestions. The most prevalent ones are making sure that there are no calculations/operations done within the controller other than calling the model/view and that you segregate the attribution functionalities well enough so users can pick what they want. Feel free to contact me if you have any questions.

openbb_terminal/portfolio/portfolio_controller.py Outdated Show resolved Hide resolved
openbb_terminal/portfolio/portfolio_controller.py Outdated Show resolved Hide resolved
openbb_terminal/portfolio/portfolio_controller.py Outdated Show resolved Hide resolved
openbb_terminal/portfolio/portfolio_controller.py Outdated Show resolved Hide resolved
openbb_terminal/portfolio/portfolio_controller.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
website/content/terminal/portfolio/attrib/_index.md Outdated Show resolved Hide resolved
website/content/terminal/portfolio/attrib/_index.md Outdated Show resolved Hide resolved
website/content/terminal/portfolio/attrib/_index.md Outdated Show resolved Hide resolved
@Sammac-dev
Copy link
Contributor Author

@JerBouma all the changes above have been committed. The only thing I'm not sure on is how to resolve the requirements.txt file. From our end we have no need for any changes to it outside of what is already on OpenBB-finance main branch. So let us know if there is anything we need to do from our end.

Copy link
Contributor

@JerBouma JerBouma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor details and then it should be finished! :)

openbb_terminal/portfolio/portfolio_controller.py Outdated Show resolved Hide resolved
openbb_terminal/portfolio/portfolio_controller.py Outdated Show resolved Hide resolved
@Sammac-dev
Copy link
Contributor Author

Sammac-dev commented Nov 10, 2022

Hi Jeroen, the requested changes have been made. Note that in order to push I had to pull first which updated the branch. "portfolio_controller.py" was the only file that I've updated but I'm unsure if there is an issue with the way I've committed (I'm fairly new to working with forks/branches). The second commit (21d4fdd) is the merge. My changes are 54f1e2b

@JerBouma JerBouma merged commit aed683f into OpenBB-finance:main Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat L Large T-Shirt size Feature portfolio Portfolio menu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants