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

Remove portfolio optimisation shortcut #2266

Merged
merged 6 commits into from
Aug 8, 2022
Merged

Remove portfolio optimisation shortcut #2266

merged 6 commits into from
Aug 8, 2022

Conversation

DidierRLopes
Copy link
Collaborator

@DidierRLopes DidierRLopes added bug Fix bug enhancement Enhancement labels Aug 7, 2022
@JerBouma
Copy link
Contributor

JerBouma commented Aug 8, 2022

Some others places po is mentioned where it shouldn't be (could you remove them too @DidierRLopes?):

  • options/screener/screener_controller.py
  • stocks/screener/screener_controller.py

Background why we remove this integration:

Portfolio optimization is not a straightforward thing. All of the parameters you can define within each po command are massive considerations that have an incredibly large impact on the portfolio. E.g. we currently have defaults set but they are not based on any financial theory that this is the right approach. Why 3y of historical data? How do returns of the past get factored in? What risk measure? What covariance matrix? What significance level? Want to go short or only long? I can go on. Simply calling maxsharpe therefore has no meaning whatsoever unless you thought of it extensively.

We do not want to give our users the impression we believe that integrations with screeners and/or comparison analysis lead to effective and/or sound portfolios. This requires 10 times much more thought than this integration indicates. Furthermore, you don't tend to optimise a non-existing portfolio in the first place.

@DidierRLopes DidierRLopes changed the title bring suffix from stocks to ca + remove po from ca Remove portfolio optimisation shortcut Aug 8, 2022
@DidierRLopes
Copy link
Collaborator Author

@JerBouma done!

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.

Looks good to me.

@DidierRLopes DidierRLopes merged commit 32c656b into main Aug 8, 2022
@colin99d colin99d deleted the bring-suffix branch August 10, 2022 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fix bug enhancement Enhancement
Projects
None yet
2 participants