-
Notifications
You must be signed in to change notification settings - Fork 24
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
Modernize portfolio optimizer #376
Conversation
Your changes were successfully integrated in the dev site, make sure to review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed the PR. Before adding general comments, I wanted to ask you the status of these tasks you wanted to complete?
Submitted first update on portfolio optimizer. Some things that I still need to do:
* added better explanations for the code especially the newer ones related to creating the panel app * remove possibly redundant math when creating the graphs
Comments after review:
- It took me a little while to get what
252
stood for, it turns out it's the number of trading days in a year. Make this a little clearer in the notebook please. - The first part of the example, that was already there, is quite dry. I don't think spending time improving that is really in the scope of our work, unless that's something that would be quick and easy for you to do!
- However, I think the introduction could be improved. At the moment it only talks about the financial aspect of the example, it could be extended a little to describe the HoloViz tools that are going to be used (hvplot, holoviews, panel) and how they support the analysis. It should also be mentioned a Panel dashboard is going to be created. Adding a screenshot of this app at the beginning would be nice (check out Isaiah's PR on
nyc_taxi
and the start of thedashboard.ipynb
notebook). - My main feedback: the section you have ported from Panel's website is also very dry. We need to add some explanation about the app we are building and how we are building it. The
Build a <app>
tutorials you have recently gone through could be a good template for this. - We should add this app as a deployment (check out
./template/anaconda-project.yaml
to see how). And if we do that, we could also wrap it in a template at the very end of the notebook, so the deployed app looks good. - Some of the elements of the app could be made responsive (i.e. set up so they dynamically stretch to occupy the available space).
- The app is built using hvPlot's interactive API. It's possible we'll try to migrate to
pn.rx
. I'm waiting for feedback from others on Discord on this point.
EDIT: I updated your original post with the modernizing checklist.
I have a suggestion about the notebook being a little dry and lacking context. Maybe you could throw the notebook content at ChatGPT and ask for it to add some brief explanations about the underlying analysis. That's just a suggestion, so feel free to ignore it if you don't feel like it. I think though that ChatGPT could do a good job at this, given this is I believe a very common kind of analysis. |
Your changes were successfully integrated in the dev site, make sure to review |
Is there anyone who is arguing for keeping it hvplot.interactive? pn.rx seems like the obvious thing to do, unless someone is very convincingly arguing otherwise. |
Was helping Jason with Also, we weren't sure how to separate out the widgets to better lay out the dashboard, unlike pn.Param. I'm -1 on using from io import BytesIO
import pandas as pd
import panel as pn
pn.extension()
@pn.cache
def get_stocks(data):
if data is None:
stock_file = 'https://datasets.holoviz.org/stocks/v1/stocks.csv'
else:
stock_file = BytesIO(data)
return pd.read_csv(stock_file, index_col='Date', parse_dates=True)
file_input = pn.widgets.FileInput(sizing_mode='stretch_width')
stocks = pn.rx(get_stocks)(file_input)
selector = pn.widgets.MultiSelect(
name='Select stocks', sizing_mode='stretch_width',
options=stocks.columns.to_list()
)
selected_stocks = stocks.rx.pipe(
lambda df, cols: df[cols] if cols else df, selector
)
pn.Column(selected_stocks) Lastly, upon printing rx objects, it raises an error |
Ok I just realized there's a section in Panel about rx; I was too focused on the param docs. To split the widget + column, you need to wrap it in the corresponding pane; personally I'd prefer a method to call to do so, e.g. from io import BytesIO
import pandas as pd
import panel as pn
pn.extension()
@pn.cache
def get_stocks(data):
if data is None:
stock_file = 'https://datasets.holoviz.org/stocks/v1/stocks.csv'
else:
stock_file = BytesIO(data)
return pd.read_csv(stock_file, index_col='Date', parse_dates=True)
file_input = pn.widgets.FileInput(sizing_mode='stretch_width')
stocks = pn.rx(get_stocks)(file_input)
selector = pn.widgets.MultiSelect(
name='Select stocks', sizing_mode='stretch_width',
options=stocks.columns.to_list()
)
selected_stocks = stocks.rx.pipe(
lambda df, cols: df[cols] if cols else df, selector
)
pn.Column(
pn.Row(file_input, selector),
pn.pane.DataFrame(selected_stocks),
) |
Your changes were successfully integrated in the dev site, make sure to review |
Your changes were successfully integrated in the dev site, make sure to review |
Can you please make a PR on Param putting in an explicit link to the Panel rx docs wherever you would have found them? Right now it only links to the main Panel site, but pointing directly to Panel rx info would presumably prevent others from having the same experience you did. |
Your changes were successfully integrated in the dev site, make sure to review |
Your changes were successfully integrated in the dev site, make sure to review |
Your changes were successfully integrated in the dev site, make sure to review |
I made a lot of changes. There were still many issues: headings, grammatical, organizational, textual. In the future, the PRs need to be further along prior to asking for a final review. |
Your changes were successfully integrated in the dev site, make sure to review |
* added panel dashboard * clear outputs * added more text description to cells * improved the introduction of the notebook * removed param.watch * cleared outputs * added more cell descriptions * fixed linter error * updated holoview plots to hvplot * removed outputs * updated yml * minor edits to made code more readable * changes to cell descriptions * updated yml files * updated yaml files again * dr revision of text code and viz * color the distribution plots --------- Co-authored-by: jtao1 <jtao@LAPTOP-UKS0PRO9> Co-authored-by: Demetris Roumis <roumis.d@gmail.com>
Modernizing an example checklist
Preliminary checks
Change ‘anaconda-project.yml’ to use the latest workable version of packages
hvplot<0.9
tohvplot
,panel>=0.12,<1.0
topanel>=0.12
) of all other dependencies. Removing the upper pins of dependencies could necessitate code revisions in the notebooks to address any errors encountered in the updated environment. Should complexities or extensive time requirements arise, document issues for team discussion on whether to re-pin specific packages or explore other solutions.hvplot
tohvplot>=0.9.2
,hvplot>=0.8
tohvplot>=0.9.2
). Usually, the new/updated lower pin of a dependency will be the version resolved afteranaconda prepare
has been run. Execute!conda list
in a notebook, oranaconda run conda list
in the terminal, to display the version of each dependency installed in the environment. Adjusting the lower pin helps ensure that the locks produced for each platform (linux-64, win-64, osx-64, osx-arm64) rely on the tested dependencies and not on some older versions.Plot API updates (discussed on a per-example basis)
datashade
withrasterize
(read this page). Essentially,rasterize
allows Bokeh to handle the colormapping instead of Datashader.Interactivity API updates (discussed on a per-example basis)
pn.interact
usage.param.watch()
usage. This is pretty low-level and verbose approach and should not be used in Examples unless required, or an Example is specifically trying to demo its usage in an advanced workflow.pn.bind()
. Read this page for explanation.view()
method and call it directly, update the class by inheriting frompn.viewable.Viewer
and replaceview()
by__panel__()
. Here is an example.Panel App updates (discussed on a per-example basis)
pn.Column
, or more complicated to incorporate widgets, etc. Make the final app.servable()
.command: dashboard
declaration in theanaconda-project.yml
file), try adding it.template = pn.template.BootstrampTemplate
, but if building up an app across multiple cells, it is probably cleaner to declare the template at the top withpn.extension(template='bootstrap')
. See how to guide on setting a template.General code quality updates
warnings.simplefilter(‘ignore’)
somewhere at the start of the notebook, remove this line. Try to update the code to remove the warnings, if any. If updating the code to remove the warnings is taking significant amount of time and effort, bring it up for discussion and we may decide to disable warnings again.Text content
Visual appearance - Example
Visual appearance - Gallery
Ml Annotators
toML Annotators
), if not, add/update theexamples_config.title
field inanaconda-project.yml
description
field inanaconda-project.yml
Workflow (after you have made the changes above)
doit validate:<projectname>
doit test:<projectname>
doit doc_one –name <projectname>
. It’s better if the project notebook(s) is saved with its outputs (but be sure to clear outputs before committing to the examples repo!) when building the docs. Then open this file in your browser./builtdocs/index.html
and check how the site looks.