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

Fix Various portfolio/po issues #3286

Merged
merged 10 commits into from
Nov 4, 2022
Merged

Fix Various portfolio/po issues #3286

merged 10 commits into from
Nov 4, 2022

Conversation

colin99d
Copy link
Contributor

@colin99d colin99d commented Nov 3, 2022

Description

Fixes #3135

  • Files can now be uploaded in params without the -f flag.
  • Refactors the load and save files to use the new paths we created including the exports folder.
  • Moves the sample files into the openbb_terminal/miscellaneous folder
  • Adds error handling when a dataframe is empty
  • Gave the autocompleter better handling for a none type
  • Updated the tr parameter to be an int
  • Add options for plot and rpf

Note: I am highly confident there are a lot more errors in this menu (it doesnt even follow MVC). And a lot of it is pretty hard to understand. However, this will take a lot of time so a decision like this goes to @Chavithra, @JerBouma, and @andrewkenreich.

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.
  • Make sure affected commands still run in terminal
    • python terminal.py portfolio/po/params/load OpenBB_Parameters_Template_v1.0.0.xlsx (can load without -f)
    • python terminal.py portfolio/po/params/load OpenBB_Parameters_Template_v1.0.0.xlsx/save colin.ini (can save to new ath)
    • python terminal.py "portfolio/po/load 50_30_10_10_Portfolio.xlsx/maxsharpe -p 1d" --debug
      • I then confirmed that the file was saved to the correct location, and that the new file matches the old file
    • python terminal.py "portfolio/po/load 50_30_10_10_Portfolio.xlsx/maxsharpe" then run maxsharpe -h
    • python terminal.py "portfolio/po/load 50_30_10_10_Portfolio.xlsx/maxsharpe -tr 5" (switched this argument to an integer)
    • Found another issue where using maxsharpe twice causes issues with prompt toolkit, code below shows a bug that is now fixed
    • python terminal.py "portfolio/po/load 50_30_10_10_Portfolio.xlsx/maxsharpe -tr 5" then maxsharpe -tr 3 then maxsharpe -tr 4
  • Ensure the SDK still works
  • Check any related reports

Checklist:

Others

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

@colin99d colin99d added the bug Fix bug label Nov 3, 2022
@colin99d colin99d marked this pull request as draft November 3, 2022 10:01
@JerBouma
Copy link
Contributor

JerBouma commented Nov 3, 2022

Much of the code is based on models from Riskfolio-lib. This is fine as we can expect the results to be accurate. However, the creator didn't follow our styling guide and wasn't the most experienced coder. So, ideally we would want to rewrite a lot of the code to improve it. I think this is relevant once we notice that there is a lot of interest from our target personas.

@colin99d colin99d marked this pull request as ready for review November 3, 2022 14:17
@jmaslek
Copy link
Collaborator

jmaslek commented Nov 4, 2022

Idk if this is an issue, but if I am in po and I run an optimization, the autocomplete no longer works for load.

load OpenBB_Portfolio_Template_v1.0.0.xlsx
maxsharpe
load {no autocomplete here}

@colin99d
Copy link
Contributor Author

colin99d commented Nov 4, 2022

This PR is a bandaid on a menu that needs a lot of refactoring. There are probably a lot of other bugs we do not know about as well, but until someone really takes the time to really fix it they will keep popping up.

@jmaslek
Copy link
Collaborator

jmaslek commented Nov 4, 2022

So the bug you addressed was the -f which is fixed.

I tested all the edge cases I had with our previous auto complete issues, and it all worked.

@jmaslek jmaslek merged commit 86f2e56 into main Nov 4, 2022
@jmaslek jmaslek deleted the fix_3135 branch November 4, 2022 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fix bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Portfolio optimization bugs
3 participants