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

Overhaul Portfolio class #2021

Merged
merged 56 commits into from
Jul 8, 2022
Merged

Overhaul Portfolio class #2021

merged 56 commits into from
Jul 8, 2022

Conversation

montezdesousa
Copy link
Contributor

@montezdesousa montezdesousa commented Jun 30, 2022

Description

The purpose of this PR is to refactor the Portfolio class that lives in portfolio_model.py and serves as base for the outputs of portfolio menu. The goal is to give it a more pythonic structure and if possible pull some logic from portfolio_view.py and portfolio_controller.py.

Following guidelines provided by @JerBouma the new Portfolio class should implement the following tasks:

  • Load orderbook
  • Load benchmark
  • Calculate trades
  • Determine reserves
  • Determine allocations
  • Determine key metrics and ratios

Summary of changes:

  • portfolio_model.py
    Rename Portfolio class to PortfolioModel
  • Load orderbook - done with logic pull from controller, now Portfolio class handles the file
  • staticmethod reads the orderbook from file (xlsx or csv)
  • PortfolioModel() object is created by receiving the previous orderbook
  • __set_orderbook method receives it and starts preprocessing
  • preprocess_orderbook() clean up orderbook and save relevant info
  • get_orderbook method returns the orderbook to call_show(), pulling logic from controller
  • Load benchmark - changed method name for consistency
  • benchmark is loaded as previously, but now it immediately calls mimic_trades_for_benchmark(). I'd suggest to remove full shares option since we are warning the user that outputs will not be 100% correct.
  • Calculate trades - previously done in generate_holdings_from_trades() renamed to generate_portfolio_data()
  • simplify structure by breaking it in subtasks
  • CASH was removed from calculations as it is not relevant for the method we're using to calculate returns
  1. load historical prices -> load_portfolio_historical_prices()
    1.1. now we have a single method to load prices instead of get_yahoo_close_prices() for stocks/etf and get_crypto_yfinance() for crypto
    1.2. tickers are kept in a dictionary by asset class instead of lists
  2. populate order book with historical prices -> populate_historical_trade_data()
    2.1. substitutes the 2 methods that previously pulled prices from yfinance
  3. calculate daily returns assuming investments are made at beginning of period t:
    Return(t) = Price(t)*Quantity(t) / [Price(t-1)*Quantity(t-1) + Investment changes]
    Return(t) = End Value(t) / Initial Value(t)
    -> calculate_value() [Only supports buy orders]
  • Determine reserves

  • Determine allocations: this task was handled by calculate_allocations(), no changes here

  • Determine key metrics and ratios: moved some metrics from helpers and view to model

  • Other files

  • portfolio_view.py
    make use of tickers list from Portfolio class to get all_holdings
    display_cumulative()_returns uses portfolio_model for cumulative_returns instead of computing them
    display_rolling_volatility(), display_rolling_sharpe(), display_rolling_sortino() use portfolio_model for rolling metrics

  • portfolio_controller.py
    solve minor bug, portfolio help menu was being prompted twice
    small fixes to adapt to new PortfolioModel class attributes (e.g. you can't create an empty portfolio in this new version)
    call_show calls display_orderbook from portfolio_view.py: logic push to portfolio_view.py to handle rich table

  • portfolio_helpers.py
    move get_sharpe_ratio(), get_sortino_ratio(), get_maximum_drawdown_ratio() to portfolio.model.py

How has this been tested?

  • Please describe the tests that you ran to verify your changes.
  • Provide instructions so we can reproduce.
  • Please also list any relevant details for your test configuration.

Checklist:

Others

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My code passes all the checks pylint, flake8, black, ... To speed up development you should run pre-commit install.
  • New and existing unit tests pass locally with my changes. You can test this locally using pytest tests/....

@montezdesousa montezdesousa added the do not merge Label to prevent pull request merge label Jun 30, 2022
@JerBouma
Copy link
Contributor

JerBouma commented Jul 4, 2022

Hey @montezdesousa, let me know when I should review :)

@JerBouma JerBouma self-requested a review July 4, 2022 07:20
@JerBouma JerBouma added feat M Medium T-Shirt size feature and removed do not merge Label to prevent pull request merge labels Jul 4, 2022
@JerBouma JerBouma marked this pull request as ready for review July 4, 2022 10:38
@JerBouma
Copy link
Contributor

JerBouma commented Jul 4, 2022

Could you also look into the calculations of cret, yret, mret and dret? E.g. the graph below is, to me, completely incorrect.

Figure_1

A portfolio with the benchmark (buying at the same moments as the portfolio) did not yield you a 300% return. This is visible when you use perf:

2022 Jul 04, 07:16 (🦋) /portfolio/ $ perf

     Portfolio vs. Benchmark - Totals in period: all     
┏━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━━┓
┃                  ┃ Portfolio ┃ Benchmark ┃ Difference ┃
┡━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━━┩
│ Total Investment │ 61331.99  │ 61331.99  │ -          │
├──────────────────┼───────────┼───────────┼────────────┤
│ Total Value      │ 73157.40  │ 82445.42  │ -9288.02   │
├──────────────────┼───────────┼───────────┼────────────┤
│ Total % Return   │ 19.28%    │ 34.42%    │ -15.14%    │
├──────────────────┼───────────┼───────────┼────────────┤
│ Total Abs Return │ 11825.41  │ 21113.43  │ -9288.02   │
└──────────────────┴───────────┴───────────┴────────────┘

2022 Jul 04, 07:18 (🦋) /portfolio/ $

What those commands do wrong is the assumption you only bought in 2010 and no other dates. If you bought everything at 2010, you probably had a 300% return by now. This is, however, not the case as we buy periodically. This is correctly reflected for the portfolio.

@JerBouma
Copy link
Contributor

JerBouma commented Jul 7, 2022

There were some merge conflicts since I merged #2029. I believe I did this right 🙏

@JerBouma JerBouma merged commit 2c3e10a into main Jul 8, 2022
@JerBouma JerBouma deleted the portfolio branch July 8, 2022 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat M Medium T-Shirt size feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants