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.

Sign up for GitHub

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

Jump to bottom

Dash UI #27

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open

Dash UI #27

wants to merge 37 commits into from

Conversation

k8culver
Copy link
Contributor

@k8culver k8culver commented Oct 2, 2024 •
edited
Loading

  • Created a user interface option for running the demo
  • Maintained the CLI run option
  • Exposed settings for period type, solver type, stocks, dates, budget, and transaction cost

demo

k8culver added 23 commits July 30, 2024 10:54
@k8culver
README updates
5b23cee
@k8culver
Dash file set up
12e5750
@k8culver
black run for legibility
ca120ea
@k8culver
Display input graph
255a308
@k8culver
Reorganizing files to match template
eff5b6b
@k8culver
Stock and date selection
dde888a
@k8culver
Banner buttons, pull stock names from yfinance
cdaa5bf
@k8culver
Add output tables for SINGLE & MULTI, move period selector to banner
0f535b3
@k8culver
Implement interval graph output
c7c0da3
@k8culver
Move helper functions to utils.py
f71f143
@k8culver
Fix init_holdings bug
9cf1aa8
@k8culver
Add stock support to dynamic solution table
8594c28
@k8culver
Update results graph to add to trace, start graph updates at i=3
1b238bb
@k8culver
Add comparison dates to slider, Add up/down arrows
c3b30be
@k8culver
Add styling for comparison tables
c1f27d2
@k8culver
Refactor multi_period.py, add docstrings, remove start-loop, add retu…
989f8ea 
…rn type hints
@k8culver
Remove problem details table, remove time limit, store settings in se…
57890b6 
…ttings-store
@k8culver
Add docstrings, clean up code
e34812d
@k8culver
Revert README
d00f357
@k8culver
Update README
b14c5b5
@k8culver
Organize callbacks, fix baseline bug
ea12cb3
@k8culver
Add help text, update preset to be original preset
35cd4fd
@k8culver
isort & black
7eed7d7
@k8culver k8culver self-assigned this Oct 3, 2024
@k8culver k8culver marked this pull request as ready for review October 3, 2024 16:20
@k8culver k8culver requested a review from thisac October 3, 2024 16:21
@k8culver k8culver marked this pull request as draft October 23, 2024 21:43
thisac
thisac reviewed Nov 27, 2024
View reviewed changes
Copy link

@thisac thisac left a comment

Choose a reason for hiding this comment

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

Looks great, and works great apart from a few issues:

  • It's not very clear that the minimum time period is 4 months (which seems to be the case). Not sure where that restrictions comes from.
  • When no feasible solution is found, an exception is raised (raise Exception("No feasible solution could be found for this problem instance.")) but the run isn't stopped. The user has to click the cancel button.
  • Multi-period seems to get stuck for some parameter choices. Might be for the same reason as above, but not sure.
  • Could perhaps be nice to have a total revenue/loss (compared to fund portfolio) in the multi-period result.

src/single_period.py Outdated

def __init__(
self,
stocks=("AAPL", "MSFT", "AAL", "WMT"),
Copy link

Choose a reason for hiding this comment

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

Maybe remove these defaults and just have it be a mandatory arg? Would need to update a couple of tests only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess that'd work as long as I keep the default in portfolio.py for command line runs

src/single_period.py Outdated
Comment on lines 514 to 522
def run(self, min_return: float = 0, max_risk: float = 0, num: int = 0, init_holdings=None):
"""Execute sequence of load_data --> build_model --> solve.

Args:
max_risk (int): Maximum risk for the risk bounding formulation.
min_return (int): Minimum return for the return bounding formulation.
num (int): Number of stocks to be randomnly generated.
init_holdings (float): Initial holdings, or initial portfolio state.
"""
Copy link

Choose a reason for hiding this comment

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

Missing return type and docstring.

src/multi_period.py Outdated

def __init__(
self,
stocks=("AAPL", "MSFT", "AAL", "WMT"),
Copy link

Choose a reason for hiding this comment

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

Remove stocks defaults?

@k8culver
Copy link
Contributor Author

  • When no feasible solution is found, an exception is raised (raise Exception("No feasible solution could be found for this problem instance.")) but the run isn't stopped. The user has to click the cancel button.
  • Multi-period seems to get stuck for some parameter choices. Might be for the same reason as above, but not sure.

What settings can I use to recreate these issues?

@k8culver
Copy link
Contributor Author

  • It's not very clear that the minimum time period is 4 months (which seems to be the case). Not sure where that restrictions comes from.

I have no idea, it was like that before I inherited it, I will do my best to try to figure out why

@thisac
Copy link

thisac commented Nov 29, 2024

What settings can I use to recreate these issues?

It happens e.g., when only having a single stock. Not sure if there are other cases, but if that might be so it's a good idea to catch this exception, cancel the run and show a note/notification that no feasible solution was found.

@k8culver
Copy link
Contributor Author

  • When no feasible solution is found, an exception is raised (raise Exception("No feasible solution could be found for this problem instance.")) but the run isn't stopped. The user has to click the cancel button.

I haven't run into this since fixing a few bugs so I haven't implemented this, but let me know if you run into it again and what settings you used.

@k8culver k8culver requested a review from thisac December 13, 2024 22:22
@k8culver k8culver marked this pull request as ready for review December 13, 2024 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierrelouisp pierrelouisp Awaiting requested review from pierrelouisp

@vgoliber vgoliber Awaiting requested review from vgoliber

@thisac thisac Awaiting requested review from thisac

Assignees

@k8culver k8culver

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@k8culver @thisac